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-exporting public modules from 'ember-animated' #90

Open
samselikoff opened this issue Mar 21, 2019 · 6 comments
Open

Re-exporting public modules from 'ember-animated' #90

samselikoff opened this issue Mar 21, 2019 · 6 comments

Comments

@samselikoff
Copy link
Contributor

I wanted to reopen the discussion about exporting EA's modules from the top-level namespace:

// current
import { fadeIn } from 'ember-animated/motions/opacity';
import move from 'ember-animated/motions/move';

// proposed
import { fadeIn, move } from 'ember-animated';

I was chatting with @twokul and @kellyselden and they said Rollup had the ability to tree-shake this style of import. I know you mentioned it is not part of the language-level module specification, but if this feature of Rollup allows us to (eventually) tree-shake packages organized like this when the time comes, what do you think about moving things over today?

@ef4
Copy link
Contributor

ef4 commented Mar 21, 2019

This is the same thing we already discussed. Rollup and webpack can both do this.

I think we should write into the v2 addon spec that all ember addons are required to be "side effect free" in the sense that this kind of transformation is safe. Then we can trust this pattern.

So ok, yes, let's design better imports. My remaining design concern is logical groupings. If we collapse things down too far people can't see what is a transition vs motion vs easing vs some other helper function provided by the library. One idea is to group into:

  • ember-animated for the things that are already on the top-level
  • ember-animated/motions to reexport all the motions
  • ember-animated/transitions to reexport all the transitions
  • ember-animated/easings to reexport all the easings

@samselikoff
Copy link
Contributor Author

Super excited to think about standardizing around this in the wider community 🎉

I had the same thought, that

import { fade, fadeIn } from 'ember-animated';

would be confusing, since one is a motion and another is a transition.

I think your grouping idea makes perfect sense! 👍

@cibernox
Copy link
Collaborator

I just learned about that spec and I think it's great.

Do you think it would be possible to craft an ESLint rule that detects exported functions with side-effects, at least some of them?

@ef4
Copy link
Contributor

ef4 commented Mar 21, 2019

We can lint for some potentially tempting Ember-specific things, like reopening a framework-provided class. But we definitely can't catch it all.

If these things were statically detectable, the whole problem would be a non-problem because instead of linting you could do the analysis in the module bundlers and they could automatically know which code is safe to strip.

@samselikoff
Copy link
Contributor Author

I understand why they are not statically detectable (otherwise it'd be part of the language spec), but in that case how do the Webpack/Rollup features work?

@ef4
Copy link
Contributor

ef4 commented Mar 21, 2019

Webpack makes npm package authors certify that they’re safe by setting a value in package.json.

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

No branches or pull requests

3 participants