chore!: use pure ES modules #577
Conversation
ehmicky
added
the
type: chore
ehmicky
force-pushed
the
chore/pure-esm
branch
13 times, most recently
from
7a81bfd to
1587661
Compare
| // it('Next.js site framework detected on load - vanilla JS site', () => { | ||
| // cy.contains('Vanilla JS Site').click() | ||
| // cy.contains('"name":"Next.js"') | ||
| // }) |
There was a problem hiding this comment.
I am hitting some issues to make those tests work. Those are testing whether the framework-info browser bundler can be loaded as a global variable. However, netlify-react-ui is loading framework-info through Webpack (which is tested by the "React site" test). Therefore, I believe we can remove those tests since they are now irrelevant. However, I am considering doing this as a separate, follow-up PR.
ehmicky
force-pushed
the
chore/pure-esm
branch
9 times, most recently
from
f6d7445 to
30a14b7
Compare
| path: path.resolve(`${__dirname}/../dist`), | ||
| filename: 'index.js', | ||
| path: DIST_DIR, | ||
| filename: 'index.cjs', | ||
| library: 'frameworkInfo', | ||
| libraryTarget: 'umd', |
There was a problem hiding this comment.
This is keeping UMD as a format for browser consumers, as opposed to using ES modules.
I initially tried with ES modules, but this created a wide array of problems when integrating it with netlify-react-ui. Those are listed in https://github.com/netlify/netlify-react-ui/issues/9869.
Keeping it as UMD is a smaller change for browser consumers, which will be simpler to integrate.
That being said, non-browser consumers will still use pure ES modules, and this repository can still use pure ES modules, which is the main goal.
| path: path.resolve(`${__dirname}/../dist`), | ||
| filename: 'index.js', | ||
| path: DIST_DIR, | ||
| filename: 'index.cjs', |
There was a problem hiding this comment.
We need to use .cjs since the top-level package.json is using type: 'module' but the bundle is not using ES modules.
ehmicky
force-pushed
the
chore/pure-esm
branch
3 times, most recently
from
957c9a8 to
3ae1857
Compare
ehmicky
force-pushed
the
chore/pure-esm
branch
4 times, most recently
from
c915f5a to
73ba8da
Compare
| "main": "./src/main.js", | ||
| "browser": "./dist/index.js", | ||
| "type": "module", | ||
| "main": "./dist/index.cjs", |
There was a problem hiding this comment.
The main field is needed for Webpack and Jest to work correctly when used by browser consumers like our front-end app. It must point to the Webpack bundle.
There was a problem hiding this comment.
shouldn't we use the browsers field instead of the main field if it is used by browsers?
https://docs.npmjs.com/cli/v8/configuring-npm/package-json#browser
There was a problem hiding this comment.
The main problem is that the frontend application is using several tools which are not quite completely ready for pure ES modules and have their own (and conflicting) interpretation of how those fields should be used: Webpack, Jest, tsc. In the future, the exports field is actually meant to replace both the main and browser fields, but those tools are not ready for it yet.
What complicates things even more is that the frontend application is run and bundled in browser mode, but when it is run by Jest, it is in Node.js mode, and will use the main field instead of the browser field.
I have spent a few hours (due to prereleases process and CI speed) trying several combinations of either main, browser or main+browser, using both ./src/main.js and/or ./dist/index.cjs and this ended up being the only combination which worked with our frontend application.
ehmicky
force-pushed
the
chore/pure-esm
branch
from
7bf1b5f to
73ba8da
Compare
ehmicky
force-pushed
the
chore/pure-esm
branch
from
73ba8da to
35764f8
Compare
cypress/plugins/index.mjs
Outdated
| // TODO: rename this file to `index.js` once Cypress supports it for pure | ||
| // ES modules | ||
| const noop = function () {} | ||
| export default noop |
| @@ -117,5 +121,8 @@ | |||
| ], | |||
| "engines": { | |||
| "node": "^12.20.0 || ^14.14.0 || >=16.0.0" | |||
| }, | |||
| "ava": { | |||
| "verbose": true | |||
There was a problem hiding this comment.
That was previously specified inside the ava config file shouldn't we add the files here as well?
export default {
files: ['test/*.js'],
verbose: true,
}
There was a problem hiding this comment.
files finds test/*.js by default in Ava, so we should be able to remove it there. The test files seem to be correctly found by Ava.
There was a problem hiding this comment.
This will be moot in ava v4 since only the verbose reporter is left IIRC :)
ehmicky
force-pushed
the
chore/pure-esm
branch
from
35764f8 to
e6b23a9
Compare
|
Fixed 👍 |
Part of #451
This switches from CommonJS to pure ES modules.
This is a breaking change, since consumers that still use CommonJS will need to use a dynamic
import()to load this module instead ofrequire(). Consumers that use pure ES modules will not need to change anything.This still works when used with browser consumers, including our front-end app (draft PR at https://github.com/netlify/netlify-react-ui/pull/9862).