Fix ESM issue for Node.js #1221
Conversation
denysoblohin-okta
force-pushed
the
od-esm-fix-OKTA-491223
branch
2 times, most recently
from
167a705 to
2017052
Compare
package.json
Outdated
| @@ -49,6 +49,7 @@ | |||
| "test:integration": "jest --config ./jest.integration.js", | |||
| "test:bundle:esm:browser": "cross-env BUNDLE_ENV=browser NODE_OPTIONS=--experimental-vm-modules jest --config ./jest.esm.mjs", | |||
| "test:bundle:esm:node": "cross-env BUNDLE_ENV=node NODE_OPTIONS=--experimental-vm-modules jest --config ./jest.esm.mjs", | |||
| "test:bundle:esm:node:app": "yarn workspace @okta/test.app.node-esm start", | |||
There was a problem hiding this comment.
is it possible to move this test to unit layer? catch error from a running node app seems like an integration test.
what if you don't map the module in jest config above?
There was a problem hiding this comment.
what if you don't map the module in jest config above?
Without module mapping in Jest yarn test:bundle:esm:bowser will fail with
SyntaxError: Unexpected token 'export'
It happens because ES module of broadcast-channel for browser (dist/esbrowser/index.js) have '.js' extension (not '.mjs').
For the same reason Jest uses special ES module of okta-auth-js for browser with .mjs extension - build/bundles-for-validation/esm/esm.browser.mjs instead of build/esm/esm.browser.js.
See
Line 93 in 7077d63
Line 16 in 7077d63
There was a problem hiding this comment.
is it possible to move this test to unit layer? catch error from a running node app seems like an integration test.
There is already unit test for validating bundles - test:bundle:esm:node
It has a little "hack" currently in master
Line 32 in 7077d63
to resolve
broadcast-chanel with ES5 version of bundle which doesn't have import/exportI've replaced it with
'^broadcast-channel$': '<rootDir>/node_modules/broadcast-channel/dist/esnode/index.mjs' This line can be removed, but then there will be problem with test:bundle:esm:browser , see comment above
There was a problem hiding this comment.
const OktaAuth = process.env.BUNDLE_ENV === 'browser' ?
`<rootDir>/build/bundles-for-validation/esm/esm.browser.mjs` :
`<rootDir>/build/esm/esm.node.mjs`;
Is it possible to follow logic here to only apply module map for browser env, and leave it to jest to figure out the node dependencies. If possible, we should avoid over testing dependencies of auth-js.
denysoblohin-okta
force-pushed
the
od-esm-fix-OKTA-491223
branch
2 times, most recently
from
ea33627 to
b464dd5
Compare
There was a problem hiding this comment.
We probably can also re-organize the bundle related tests to put all of them in one single bacon task, so we can include anything that make sense to simply verify different version of bundles.
The idea above should not be included in a patch version release (no change will be needed).
added test:bundle:esm:node:app
This reverts commit 4e3838f.
denysoblohin-okta
force-pushed
the
od-esm-fix-OKTA-491223
branch
from
b464dd5 to
9bc0cf3
Compare
Codecov Report
@@ Coverage Diff @@
## 6.6 #1221 +/- ##
=======================================
Coverage 93.41% 93.41%
=======================================
Files 157 157
Lines 4586 4586
Branches 1058 1058
=======================================
Hits 4284 4284
Misses 283 283
Partials 19 19 Continue to review full report at Codecov.
|
OKTA-491223 <<<Jenkins Check-In of Tested SHA: d2d5d62 for [email protected]>>> Artifact: okta-auth-js Files changed count: 5 PR Link: #1221
This PR resolves issue #1188
Issue:
okta-auth-jscan't be loaded as ES module for Node.js because of incorrect ES bundle ofbroadcast-channelbroadcast-channelto4.13.0(contains Fix ES module for Node.js pubkey/broadcast-channel#962 and ESM: use .js extension with type: module in package.json pubkey/broadcast-channel#972)test:bundle:esm:node, not needed nowInternal ref: OKTA-491223