-
Notifications
You must be signed in to change notification settings - Fork 533
Add support for ES modules (work in progress) #489
base: master
Are you sure you want to change the base?
Conversation
@nolanlawson nice work! This is a clever solution to a problem that we've wanted to address for some time. As for defining properties in this scenario I don't immediately see any better options than passing them into the functions when called, which as you mention is a breaking change. I'll be doing some testing with your proposal over the next couple of weeks with a focus on an alternative for those object properties and hopefully come up with some options. |
Are you concerned that we'd be better off keeping the old behavior and not doing a major version bump? If so, I'd be happy to go back and investigate to see if we can maintain the old behavior while still supporting a new ES module API. I suspect just bumping the major version may be easier though. |
The issue now seems to be a merge conflict; I'll look into this… |
2f40ca7
to
8b6c78e
Compare
OK, I very carefully rebased on top of the latest master (648da99). I note that there seem to be two variables, This PR also contains the new test for |
Addressed the broken tests in #498 |
Hm okay, tried to write an ES module format that could produce both a default export with booleans that could be toggled on and off, and named exports for tree-shaking, but seems Rollup complains when you do this:
My vote would be to just go with adding the boolean options to each API and then releasing it as a breaking change. |
Ping... wondering if there are some improvements I could make to make this PR more palatable:
Do those sound good @caseyahenson? |
@nolanlawson the big thing we want to avoid here is a major version bump. We could use default parameters within the predefined functions so that backwards compatibility is maintained, but the properties would need to be passed into the functions to overwrite defaults so I believe that would still be considered a breaking change. Can you elaborate a bit on the idea of shipping both ES and CommonJS modules? |
The idea would be to ship a |
No more update about this important feature ? |
This PR isn't fully baked yet. The point is to start a conversation. 😃
Background
I've been looking into shrinking the size of the
emojione
JS bundle. The context is that I've been analyzing the performance of the Mastodon webapp, and it appearsemojione
is roughly 1/3 the size of the entire app bundle and may contribute as much as 1 full second to the load time (on mobile devices in particular).One way to make the
emojione
codebase "smaller" without much extra work is to support named exports via ES modules. Then consumers can use a module bundler like Webpack or Rollup to include only the parts of the API that they need (tree-shaking).Improvement
What this PR does is divide the existing codebase into a few small source files (mostly for readability), then build
emojione.js
as a UMD build andemojione.es.js
as an ES module build using Rollup. Then it points to the ES module build frompackage.json
as"module"
, which both Webpack and Rollup support.I tested out the size improvements with this PR, and it works! In this PR,
emojione.js
is 932429 bytes / 566902 gzipped, whereas just doingimport { shortnameToUnicode } from 'emojione'
results in 610828 bytes (34% smaller) / 380337 gzipped (32% smaller).FWIW, you can compare this to 565800 bytes for the current production version of
emojione.min.js
, so note I also managed to make it slightly smaller even for CommonJS/globals users, mostly due to removing repeatedns.foo = foo
statements and using inline variables instead.Next steps
There's only one catch: there are some parts of the API that I couldn't faithfully reproduce in both CommonJS/UMD format and ES module format. That's these ones:
greedyMatch
imageTitleTag
sprites
unicodeAlt
ascii
For these options, the user is supposed to toggle the boolean and the relevant functions update themselves automagically. I could probably write a Rollup plugin to do this correctly for the CommonJS output (ES modules have a concept of variable binding and so it's not needed there), but another option I considered was to simply add these as input parameters to the functions that need them, which to me is more explicit anyway. But unfortunately that would be a breaking change and so would require a major version bump (per SemVer).
What do you think of these two options, and of this PR in general? Looking forward to helping improve
emojione
's perf! 😄