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

Esnext esm rollup, optionally w/o EventTargetShim #107

Closed
wants to merge 1 commit into from

Conversation

nykula
Copy link

@nykula nykula commented May 15, 2020

Split cjs,esm plugin rollup config. For esm, override tsconfig target compiler option with esnext. For both, replace EventTargetShim d.ts import with DOM EventTarget in the generateBundle hook. Move event-target-shim back to build dependencies.

If EVTSHIM=0 rollup, use native EventTarget only without importing event-target-shim or inlining it in the js. Dummy EventTarget=Object if undefined, so that serverside import doesn't crash (I split fetch and render, not using events there). This results in 30% smaller file, winning TTI milliseconds on embedded devices where I build engines from source and so know they're modern enough.

Size of esm is 184k before (commit 003d2c2), 152k after, 92k with EVTSHIM=0. Resolves #65. Please build and try this in some real-world app copy before merging, because my app is still Preact-based, I only ran this against the few hundred lines of small components I could port to Crank quickly.

@brainkim
Copy link
Member

Hmmm the big problem with optionally requiring event-target-shim is that safari just doesn’t have a working EventTarget class right now 🙃 I think the best solution is to leave it as is and write a clean room implementation of event targets which also implement features like custom capturing/bubbling and event delegation. Lots of work to do!

The other optimizations will definitely be required and I’m planning to get it all in in a 2.0 release. Thanks for trying Crank! Let me know your thoughts in a discussion or here or whatever.

@ryhinchey
Copy link
Contributor

We should be able to keep the other parts of this PR that don’t touch event target shim. @brainkim i agree we should keep that as is for now but transpiling the esm build with an esnext target makes sense to me

@brainkim
Copy link
Member

Closing in favor of #108. I do want to remove event-target-shim from our dependencies, but it is not gonna happen yet.

My current thinking is that we’re going to ship ESNext everywhere because it’s better for performance and if your environment doesn’t support async/generator functions anyways you’re gonna have a hard time writing Crank components to begin with, and as a framework we can require transpilations of dependencies for older environments.

Thanks for the effort though! I do plan on making some changes to reduce crank bundle size soon!

@brainkim brainkim closed this May 17, 2020
@nykula
Copy link
Author

nykula commented May 18, 2020

@brainkim @ryhinchey Ok! I appreciate the review.

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

Successfully merging this pull request may close these issues.

Esnext esm/* target, rather than es5?
3 participants