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

re-add esm bundle, and switch extension to module.js instead of mjs #2420

Closed
fuzetsu opened this issue May 30, 2019 · 6 comments
Closed

re-add esm bundle, and switch extension to module.js instead of mjs #2420

fuzetsu opened this issue May 30, 2019 · 6 comments

Comments

@fuzetsu
Copy link
Contributor

fuzetsu commented May 30, 2019

Description

In mithril 2.0.0-rc.5 the ESM bundle was removed (#2366). This was done mainly because of issues with webpack that caused confusion among users. Webpack would use the mjs file instead of the js file and not expose the default export in the standard way.

const m = require('mithril').default

preact had a similar issue because they also provided an mjs version of their bundle on npm. In their 10.0 beta they resolved this issue by changing the file extension to module.js which does not cause issues with webpack.

Why

Considering that most competing frameworks/libraries offer such a bundle, new users are likely to expect it, and even though using a script tag continues to work fine it's nice to have the option to use native modules when writing an app without a build process.

Other frameworks' bundle:

Possible Implementation & Open Questions

This is the commit that fixed the issue for preact. I propose we follow suit, and just change the extension to module.js.

preactjs/preact@8ea29b5

Do we want to continue to use our custom scripts to generate the esm build?
Are we ready for rollup or something similar? (maybe too much change at this point) #1886

Is this something you're interested in working on?

Yes

@dead-claudia
Copy link
Member

Could you find some relevant discussion on Preact's side?

Also, I consider this a subbug of #1886, not a strict duplicate.

@fuzetsu
Copy link
Contributor Author

fuzetsu commented May 31, 2019

@isiahmeadows

There doesn't seem to be a huge amount of discussion specifically about switching from mjs to js. but quite a bit around the issues pre switching.
My impression is that getting rid of the module bundle was not an option for them, and changing the extension was an acceptable workaround, so they just went with it when they found it. There might be some behind the scenes discussion I'm not finding.

Here are few comments/isses that mention the switch:
preactjs/preact#1424 (comment)
preactjs/preact#1321
preactjs/preact#1425
preactjs/preact-render-to-string#86 (comment)

I also checked their slack, but wasn't able to find many references to this topic there.

@dead-claudia
Copy link
Member

Okay, it appears they've been pointing people to this Webpack plugin as a workaround, and we apparently weren't the only ones running into inconsistent module resolution logic with Webpack. (We may have just ran into a different variant of the same greater problem.)

I'm open to this, and feel free to send a PR. Here's generally what I'm thinking:

  • The bundle would work mostly like how it did previously.
  • mithril/index.module.js would work like mithril/index.js, but 1. export everything as named exports, including mithril/render/hyperscript.js as m and 2. not attach everything to a hyperscript proxy to re-export as default. (We want to push people to use the named exports where applicable, to maybe save some bundle size.)
  • The other top-level modules would work like export * from "./render.js" for mithril/render.module.js and similar. I don't want these to be magic, and I'd rather not duplicate implementation. We can just continue to instruct Rollup users to install rollup-plugin-commonjs, since many Mithril libraries already require this. In the future, this might change if/when we migrate the core code base to ES6, but it's just kicking the can down the road.

@fuzetsu
Copy link
Contributor Author

fuzetsu commented Jun 10, 2019

Sound good! 👍
I'll try to submit a PR this week, hopefully tomorrow.

@backspaces
Copy link

Make sure to include a "module:" property in package.json so that unpkg, pika and other CDNs can serve the module easily.

@dead-claudia dead-claudia moved this to Low priority in Triage/bugs Sep 2, 2024
@dead-claudia dead-claudia closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2024
@github-project-automation github-project-automation bot moved this from Low priority to Closed in Triage/bugs Sep 2, 2024
@dead-claudia
Copy link
Member

Closing due to age.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

No branches or pull requests

3 participants