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

output mithril, stream and ospec esm versions on build - fixes #2112 #2194

Merged
merged 18 commits into from Nov 14, 2018

Conversation

8 participants
@porsager
Copy link
Contributor

porsager commented Jul 21, 2018

This adds an extra build step which will create esm builds of mithril, stream and ospec.

It also adds a "module" field to package.json's such that eg. module use through unpkg.com will work, and bundling with rollup won't require rollup-plugin-commonjs.

For instance flems usage would work like this:

import m from 'mithril'
import { stream } from 'mithril
import o from 'ospec'
import stream from 'mithril/stream'
import m, { stream } from 'mithril'

The reason it's done simply by rewriting the output is because my understanding is that mithril would change the src to esm eventually, so I didn't want to spend too much time getting caught up in the mithril bundler.

Motivation and Context

fixes #2112

How Has This Been Tested?

Manually

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@porsager porsager requested a review from tivac as a code owner Jul 21, 2018

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Aug 6, 2018

@porsager thanks for the PR!

@spacejack how does this jive with the TS typings?

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Aug 6, 2018

Ah... good question. I guess it depends on what the bundler does, I'm not sure how they will all behave.
If they pick up the module field in package.json then the types should be updated to es exports.
I guess the individual modules (render, request, etc.) are still commonjs?

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Aug 6, 2018

@spacejack what kind of configuration variables should we be looking at?

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Aug 6, 2018

Quick overview.

Mithril's types currently describe it as a commonjs module.

Since TS 2.7 you can use import m from 'mithril' even though it's a commonjs module, using a regular commonjs bundler, if you set Typescript's esModuleInterop compiler flag. Some bunders might do this for you however (rollup) with their commonjs plugin, in which case you'd omit that TS compiler flag.

If bundlers pick up the new module property from package.json and use the esm file, then we'll need to update the types to describe the module as having ES exports.

The separate modules - render, request, hyperscript, etc. look like they'd remain commonjs? Should they have esm versions as well?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Aug 6, 2018

@spacejack I expected it would be a headache, and I have absolutely no idea of how to make it all fit together :-)

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Aug 6, 2018

So I've run a quick test with browserify, installing posager's esm-output branch.

As far as I can tell, it doesn't affect or break old builds. I'm guessing this is because I've specified commonjs modules in typescript's tsconfig (which 99.999% of TS projects likely do.)

I'm hoping that changes to the d.ts could be deferred until later, falling back on commonjs until we add support for esm in the typedefs. (i.e., Typescript use cases wouldn't hold up this change.)

Ideally this would be tested with webpack, rollup, parcel, etc. Unfortunately using esm is something I haven't looked into at all yet.

@porsager might be helpful to provide a branch with pre-built esm files we could easily install.

EDIT: Thinking about this... I wonder if this affects any projects that aren't attempting to use ESM across the board when bundling. Do any bundlers conditionally check each module and attempt to use esm when present?

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Aug 6, 2018

Typescript issues aside, should we maybe wait on converting all the modules to esm before going ahead? As it is, you'd be able to do import m from 'mithril' but then need to handle cjs to do import h from hyperscript. I realize that's probably a niche use case but some users might do it.

@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Aug 6, 2018

Yeah, this PR was exactly to have a point inbetween now, and converting the entire thing to esm.
I think the important thing now is to not expose things that might change with the overall conversion. @tivac has taken a stab at converting the entire thing to esm a couple of times, so it would be great to have him weigh in if he thinks the current setup looks ok.

My point on gitter wrt. the various parts being exposed at some point was just to have it included in the considerations if removing export { default as stream } from "./stream/stream.esm.js". I don't see why we shouldn't expose stream as a named export if it's doable with the types?

I've added the bundled files now, so we should be able to test various setups fairly easily.

npm install porsager/mithril.js#esm-output
@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 6, 2018

@porsager Yeah I've converted it at least twice. Probably time to do it for real (& switch to rollup because the tiny filesize benefit to bundler isn't worth the maintenance/brain-space cost), but I'm not even currently using mithril so my available time is pretty limited for that work.

The good news is that the process isn't too hard, the tricky part is the way that all the current tests depend on using bundler in the browser. That'd need to change to only testing the bundle which is gonna mean not using mithril internals within tests.

@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Aug 7, 2018

@tivac Yeah I just wanted to make a PR to make hyperscript merge class and className if both were defined, and it wasn't the actual code, but rather changing the tests that made me give up on it :-(

Would you have m as default export and stream as a named export when you did the conversion too?

@spacejack has also done some testing to see how this PR works with typescript here https://github.com/spacejack/mithril-esm-tests

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Aug 7, 2018

@porsager I've been there already.

The way class and className are handled is deliberate. Otherwise you can run into issues when using both a class selector and either a class or a className attribute intermittently.

Diffing from {class: "foo"} to {className: "bar"} would set className to "bar", then notice the absence of class and remove it. Maybe that could be solved by removing attrs first then setting the new ones, though... We'd have to think through how nullish attrs would behave.

The normalization could be made while diffing attrs, but it would have a perf impact, even for folks who use m.Vnode and only one kind of attribute, manually. So keeping normalization at the hyperscript level makes sense to me (and I suppose that's why @lhorie did it like that initially).

@tivac I actually like the way testing is set up, especially for the core router. If we end up ditching the module polyfill, I think we should expose the factories.

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Aug 7, 2018

Ran through some tests with browserify, webpack, rollup and parcel. All still appear to build ok using the current type defs. All but browserify use the .esm files in the bundle; browserify ignores them.

webpack and parcel include stream in the bundle even if you're not using it (as do browsers apparently, when using the modules in script tags.) So I think I'd prefer not to have stream exported from the main mithril bundle.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 7, 2018

Would you have m as default export and stream as a named export when you did the conversion too?

I made m the default export and confined stream to its own bundle, iirc.

@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Aug 7, 2018

Ok goodie :) I'm fine with removing stream from 'mithril' for now.. It's easy to add later if we want it..

porsager added some commits Aug 8, 2018

Fix #1714 conditionally halting stream (#2200)
* Fix #1714 conditionally halting stream

* Add note in changelog
@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Oct 25, 2018

Also curious if this wouldn't be nice to have included for the 2.0 release?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 25, 2018

@porsager Maybe. It's non-breaking in its current form, so it doesn't have to ship with v2.0.0 to make it in v2. I do have a few requests:

  • Could you re-export by name all the m.* methods?
  • Could you re-export the hyperscript factory by name as m.m and change m itself in the index file to just be function m(elem, attrs) { return hyperscript.apply(this, arguments) }? It'd slightly increase the overall bundle size, but it'd set up the stage for me to transparently shift from this to actually using Rollup and allowing full tree shaking support.
@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Oct 25, 2018

Certainly. Sounds good. I'll do it first thing tomorrow morning.

@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Oct 26, 2018

@isiahmeadows Was these changes wrt. m.m and a named m export what you had in mind?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 26, 2018

@porsager Yes. Basically, separating it out into this:

var hyperscript = require("./hyperscript.js")
function m(elem, attrs) { return hyperscript.apply(this, arguments) }
m.m = hyperscript

Three more nits that I forgot to mention:

  1. Could you use .mjs instead of .esm.js/.esm.min.js? That way, Node can still correctly read it.
  2. Could you modify the bundler to rename locals with those export names, so we can still use them elsewhere?
  3. Go ahead and git checkout next -- mithril.js mithril.min.js, so those don't pollute the change list and cause conflict.
@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Oct 26, 2018

  1. I'd be a bit sad to go the .mjs route already.. I personally don't think it's the right solution for node, and it's still not set in stone.

  2. Not really sure what you mean here?

  3. Ah yes.. Thanks :)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 26, 2018

@porsager

  1. I'd be a bit sad to go the .mjs route already.. I personally don't think it's the right solution for node, and it's still not set in stone.

Webpack, Rollup, and Parcel all three expect .mjs for ES modules, .js for ES scripts, following the same steps Node is. And FWIW, .mjs is mostly a settled discussion now - it's just taken until a few months ago for people there to get this far. (The main concern is CJS interop.)

Worst case scenario, we just ship a few more files and/or package.json changes in minor releases, ripping out the old stuff in the next major update. I'm not that concerned.

  1. Not really sure what you mean here?

The bundler currently renames locals as necessary so they don't conflict with similarly-named variables in other modules. But since exporting them like that creates new bindings (whose names are actually significant), the bundler needs to rename all existing locals with their names so you don't have weird issues like render() invoking m.render instead of a locally defined function, for example.

Could you modify the bundler to rename locals with those export names, so we can still use them elsewhere?

@isiahmeadows isiahmeadows added this to Under consideration in Feature requests/Suggestions via automation Oct 28, 2018

@isiahmeadows isiahmeadows moved this from Under consideration to In discussion in Feature requests/Suggestions Oct 28, 2018

@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Nov 8, 2018

With regards to 1, I've already spent too much time trying to get to grips with it, and it's not a thing I want to get further into, so I've changed it now ;)

Wrt. 2, I'm afraid trying to fix it in the bundler might be a timesink. How would you feel about something like this instead?

export default m
var _m = m,_trust = m.trust,_fragment = m.fragment....
export {_m as m,_trust as trust,_fragment as fragment....
@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 9, 2018

@porsager Could you revert/remove the generated files from your diff? Travis will auto-generate them after your patch anyways, and they have a tendency to generate conflicts in PRs.

With regards to 1, I've already spent too much time trying to get to grips with it, and it's not a thing I want to get further into, so I've changed it now ;)

What you've got is fine now. So no worries here. 😄

Wrt. 2, I'm afraid trying to fix it in the bundler might be a timesink. How would you feel about something like this instead?

I'd be fine with that; just make sure to leave a comment why, so it doesn't get incorrectly "optimized" out.

Feature requests/Suggestions automation moved this from In discussion to Planned/In Progress Nov 14, 2018

@isiahmeadows isiahmeadows merged commit c3896b9 into MithrilJS:next Nov 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature requests/Suggestions automation moved this from Planned/In Progress to Completed/Declined Nov 14, 2018

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Nov 14, 2018

Update the changelog
- I missed that part when reviewing MithrilJS#2194
@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 14, 2018

BTW, I forgot to prompt you to add a changelog entry, so I did it for you. Sorry about that.

Also, I'm at a complete loss as to why Travis didn't think to trigger a build when I merged this... 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment