-
Notifications
You must be signed in to change notification settings - Fork 64
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
A few issues noticed as a result of a gulp attempt #200
base: master
Are you sure you want to change the base?
Conversation
Responses to your question:
Overall, can you suggest why you want to move to ESM? I'm not up to speed on its adoption. I do think the work I'm doing to build for browsers is likely unnecessary. I've started a few new JavaScript projects and I don't use Gulp, nor do I do any sort of Browser-specific builds anymore. I'm kinda out of the loop as far as best practices but I'm all for migrating to the best practices. |
Ok.
There was an outstanding bug regarding discovery of functions around
Because it is becoming the standard both for browser and server (Node, Deno, etc.), it is compelling to do so now. A popular maintainer of Node packages, sindresorhus, has, for example, been moving packages to ESM-only even: https://medium.com/sindre-sorhus/get-ready-for-esm-aa53530b3f77 . Packages that wish to use such packages must be ESM. Besides this reason, there is the fact that for browsers (and I believe Deno), the only modular approach available without transpilation or use of a framework is ESM.
I'm a fan of browser builds. To me, not providing one is like not providing binaries. It might not be strictly necessary, but it's really cumbersome for the average consumer not to have one. This is especially a problem if one's build process would require the extra work of transpiling CommonJS into ESM (i.e., if sources are not already all ESM)—as is the case here. It requires the consumer to be aware of one's implementation details. Something the modularity of ESM is meant to avoid. While some will argue that ESM is not needed in the browser, with the goal being to have a single bundle that avoids extra HTTP requests where it is necessary to wait for the results to find additional imports, for the purposes of simple (and commonly needed) repo demos, it can be easier and cleaner to take advantage of the modularity of ESM. And again, even if the consumer does wish to transpile (e.g., being keen to build with the latest dependency versions), they ideally won't need to be bothered by precise details of the build process (such as needing to know to introduce Node resolution and CommonJS plugins to their Rollup routine if the dependencies aren't already all ESM). |
I defer to your judgement. I just want to make it as easy to build/release as possible using best of breed approach in doing so. As for consumption, same thing. |
Unfortunately, after getting most things in order for the proposed ESM PR, it seems the Rollup ecosystem for polyfilling Node in the browser does not work out of the box as it does with Webpack. There are packages like https://github.com/ionic-team/rollup-plugin-node-polyfills but errors are still coming through and although I made some progress working around them, some of the issues seem insurmountable without forking the polyfills project. I'd still recommend merging this PR, however. |
overrides
to avoid error on higher Node versionsRunning the
gulp
script was giving the error:...which as per https://stackoverflow.com/a/58394828/271577 can be fixed by adding an
overrides
.There were also a few linting errors, also fixed.
The browser tests still don't run, however.
I do have some energy at the moment, so am looking to make another, updated PR for ESM support, but have a couple questions on that:
engines
inpackage.json
to the LTS versions of Node? That should let us also have the binary in ESM without transpilation.