Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a hybrid module for node #1132

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

avifenesh
Copy link
Collaborator

@avifenesh avifenesh commented Mar 17, 2024

Fix for issue 1116:
Some of the client of our package are using Ecma and some Commonjs as they're JS standard.
In order to make it available in both standards, iv'e created an hybrid build, in which the user will get the build fits his need base on his standard.

Added tests of basic usage (the main test that is needed is to see whether its possible to use the built package in the JS projects of different standard type).

Added the tests to the Node workflow.

Changes explanation in a bit more details:
We are using TSC in order to translate our package to JS.
So instead of using one file of tsconfig.json for the rules of the translation we are using a hybrid method -
tsconfig-base as the general tsconfig file, which the other will inherited from.
tsconfig-cjs for commonJS build with commonjs as the target of translation and and tsconfig for ECMA build with ECMA as the target of translation.

While running build we run two different builds - one will use tsconfig and will translate the package into build-ts/mjs, and the other will use tsconfig-cjs and will translate the package into build-ts/cjs.

In our package.json we are adding the rule -

    "exports": {
        ".": {
            "import": "./build-ts/mjs/index.js",
            "require": "./build-ts/cjs/index.js"
        }
    },

In which depends on what type of import statement the user choose (import in ECMA, and require in commonJS), the user will get the fitting package to his standard.

As part of the build we are running the fixup_pj_files_for_build_type script -
This script add "type" and "types" values to the different package.json that been created for mjs and cjs with the fitting values.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@shachlanAmazon
Copy link
Contributor

please add to the PR description (or into DEVELOPER.md) an explanation of the how, not only the what. As it is, the PR is a collection of scripts, and it's very hard to understand how the pieces combine and what they do.
And, of course, fix CI.

@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch from a08af09 to 0a56c85 Compare March 17, 2024 16:51
@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch 2 times, most recently from 2a80ea9 to d7f7389 Compare March 17, 2024 17:19
@avifenesh
Copy link
Collaborator Author

please add to the PR description (or into DEVELOPER.md) an explanation of the how, not only the what. As it is, the PR is a collection of scripts, and it's very hard to understand how the pieces combine and what they do. And, of course, fix CI.

Added explanation for both PR and developer.md, please let me know if the explanation is clear and helpful @shachlanAmazon

@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch from d7f7389 to 7f025b1 Compare March 17, 2024 17:22
Copy link
Contributor

@shachlanAmazon shachlanAmazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work!
@barshaul thoughts?
IMO we should release a version after this will be merged.

node/DEVELOPER.md Outdated Show resolved Hide resolved
node/DEVELOPER.md Outdated Show resolved Hide resolved
node/DEVELOPER.md Outdated Show resolved Hide resolved
node/npm/glide/tsconfig.json Outdated Show resolved Hide resolved
node/DEVELOPER.md Outdated Show resolved Hide resolved
.github/workflows/node.yml Outdated Show resolved Hide resolved
node/hybrid-node-tests/Ecma-test/package.json Outdated Show resolved Hide resolved
node/hybrid-node-tests/Common-test/package.json Outdated Show resolved Hide resolved
node/hybrid-node-tests/Common-test/package.json Outdated Show resolved Hide resolved
node/npm/glide/tsconfig-base.json Outdated Show resolved Hide resolved
@shachlanAmazon
Copy link
Contributor

please also update the CHANGELOG.md file

@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch 3 times, most recently from 852f4f0 to a1591cb Compare March 18, 2024 10:02
@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch 3 times, most recently from 6e1fbbc to 4e04973 Compare March 18, 2024 10:50
@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch from 4e04973 to 4a890e3 Compare March 18, 2024 10:56
node/package.json Outdated Show resolved Hide resolved
node/DEVELOPER.md Outdated Show resolved Hide resolved
node/DEVELOPER.md Outdated Show resolved Hide resolved
@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch from d8a0f62 to e8405c9 Compare March 18, 2024 11:30
Copy link
Collaborator

@barshaul barshaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch from e8405c9 to 4b89b95 Compare March 18, 2024 11:33
Copy link
Contributor

@shachlanAmazon shachlanAmazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

node/DEVELOPER.md Outdated Show resolved Hide resolved
node/hybrid-node-tests/commonjs-test/commonjs-test.cjs Outdated Show resolved Hide resolved
@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch from 4b89b95 to 0539b16 Compare March 18, 2024 12:48
@avifenesh avifenesh force-pushed the Features/Node/HybridNPMModule branch from 0539b16 to 739ebf4 Compare March 18, 2024 12:50
@avifenesh avifenesh merged commit b7833a9 into valkey-io:main Mar 18, 2024
6 checks passed
@avifenesh avifenesh deleted the Features/Node/HybridNPMModule branch March 18, 2024 13:15
node/hybrid-node-tests/commonjs-test/package.json Outdated Show resolved Hide resolved
.github/workflows/node.yml Show resolved Hide resolved
.github/workflows/node.yml Show resolved Hide resolved
avifenesh added a commit to avifenesh/valkey-glide that referenced this pull request Mar 18, 2024
avifenesh added a commit to avifenesh/valkey-glide that referenced this pull request Mar 18, 2024
Sa1Gur pushed a commit to Sa1Gur/glide-for-redis that referenced this pull request Mar 18, 2024
* Added a hybrid module for node

* Added explanation to DEVELOPER.md about the hybrid build process

* PR fixes - DEVELOPER.md wordings, Script naming, CI structure, tsconfig usage in npm folder, enter to change log

* added tsconfig property to jest

* Added comment to fixup script file
avifenesh added a commit that referenced this pull request Mar 19, 2024
avifenesh added a commit to avifenesh/valkey-glide that referenced this pull request Mar 19, 2024
* Added a hybrid module for node

* Added explanation to DEVELOPER.md about the hybrid build process

* PR fixes - DEVELOPER.md wordings, Script naming, CI structure, tsconfig usage in npm folder, enter to change log

* added tsconfig property to jest

* Added comment to fixup script file
avifenesh added a commit to avifenesh/valkey-glide that referenced this pull request Mar 19, 2024
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024
* Added a hybrid module for node

* Added explanation to DEVELOPER.md about the hybrid build process

* PR fixes - DEVELOPER.md wordings, Script naming, CI structure, tsconfig usage in npm folder, enter to change log

* added tsconfig property to jest

* Added comment to fixup script file
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants