feat: improve compatibility with native ESM in Node and the browser #21373
Conversation
|
@guybedford could you please take a look and make sure the changes I propose are indeed correct from the Node's and browser ESM behaviour's standpoint |
vovacodes
force-pushed
the
feature/improve-esm-compatibility
branch
2 times, most recently
from
ecb129d to
372c0df
Compare
There was a problem hiding this comment.
This is the correct interop pattern per the Node.js CJS / ESM interop semantics - https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#commonjs-namespaces.
web3.js/package.json
Outdated
| @@ -55,6 +55,7 @@ | |||
| "re": "semantic-release --repository-url git@github.com:solana-labs/solana-web3.js.git", | |||
| "test": "mocha -r ts-node/register './test/**/*.test.ts'", | |||
| "test:cover": "TS_NODE_COMPILER_OPTIONS='{\"module\": \"commonjs\" }' nyc --reporter=lcov mocha -r ts-node/register './test/**/*.test.ts'", | |||
| "test:esm-compat": "node test/esm-compat.mjs", | |||
There was a problem hiding this comment.
Perhaps add this invocation to the CI definitions as well?
There was a problem hiding this comment.
good point, added
web3.js/test/esm-compat.mjs
Outdated
| // This tests if the package is configured correctly and can be imported in Node versions that support ESM. | ||
| import * as web3 from '@solana/web3.js'; | ||
|
|
||
| console.log(web3); |
There was a problem hiding this comment.
While this is a good test when looking at it, I don't think it will fail the CI if it fails. Try using mocha framework describe it like the other tests, so that it fails when we test through mocha.
There was a problem hiding this comment.
It should fail CI because Node will just throw an exception if it cannot import the module, I can make it a mocha test but it should fail in exactly the same way, no describe or it blocks will ever be reached if that happens
There was a problem hiding this comment.
Using a dynamic import might work, ok let me try that...
There was a problem hiding this comment.
I see you added a different step in the CI. I think that should cover it for failure. Just using mocha would keep it standardized across other tests vs just testing if you can console.log web3. Looks like something needs to be updated unrelated to the PR to fix the build issue. Will wait for another reviewer to get their eyes on this.
There was a problem hiding this comment.
Makes sense, I'll update
There was a problem hiding this comment.
Had to remove the test as it turns out there's no way to test ESM compatibility without either changing the build files to .mjs or switching "type" to "module" in package.json. I'm happy to do it if there's willingness from the authors to accept a change that potentially might break compatibility with some legacy bundlers
|
To be honest, the fact that this PR doesn't alter the existing tests from passing should be enough since interop is already tested in the current browser bundling, it's just a slight alteration to the pattern. |
jstarry
changed the title
mergify
bot
removed
the
automerge
|
automerge label removed due to a CI failure |
vovacodes
force-pushed
the
feature/improve-esm-compatibility
branch
from
6cb3bec to
7ce38a4
Compare
jstarry
changed the title
|
Thanks for fixing the commit message, I just added a way to override this for next time: #21414 |
Codecov Report
@@ Coverage Diff @@
## master #21373 +/- ##
===========================================
- Coverage 81.5% 70.2% -11.4%
===========================================
Files 500 35 -465
Lines 140604 2066 -138538
Branches 0 295 +295
===========================================
- Hits 114704 1451 -113253
+ Misses 25900 515 -25385
- Partials 0 100 +100 |
|
You sent me too many emails. I need to read them one by one before I can reply to you. It takes some time. Wait a minute.
------------------ 原始邮件 ------------------
发件人: "solana-labs/solana" ***@***.***>;
发送时间: 2021年11月24日(星期三) 晚上10:20
***@***.***>;
***@***.******@***.***>;
主题: Re: [solana-labs/solana] feat: improve compatibility with native ESM in Node and the browser (PR #21373)
Codecov Report
Merging #21373 (2c20266) into master (c3e5927) will decrease coverage by 11.3%.
The diff coverage is n/a.
❗ Current head 2c20266 differs from pull request most recent head 7ce38a4. Consider uploading reports for the commit 7ce38a4 to get more accurate results
@@ Coverage Diff @@ ## master #21373 +/- ## =========================================== - Coverage 81.5% 70.2% -11.4% =========================================== Files 500 35 -465 Lines 140604 2066 -138538 Branches 0 295 +295 =========================================== - Hits 114704 1451 -113253 + Misses 25900 515 -25385 - Partials 0 100 +100
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Problem
This package is failing in the browser ESM + Import Map environment because of an incorrect use of the
js-sha3package, which is a CJS package and doesn't provide any named exports.Summary of Changes
This PR also adds a test that ensures ESM compatibility in the future.