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

Remove .default & .resetIdCounter from index.js #380

Closed
Andarist opened this issue Mar 20, 2018 · 3 comments
Closed

Remove .default & .resetIdCounter from index.js #380

Andarist opened this issue Mar 20, 2018 · 3 comments
Milestone

Comments

@Andarist
Copy link
Collaborator

Andarist commented Mar 20, 2018

Problem description:
Such static property assignments prevent tree-shaking (which is still relevant in case of downshift in case it gets imported by some some unused part of the code, i.e. some ui-kit library which index.js reexports bunch of stuff):

https://github.com/paypal/downshift/blob/d2df6ec05198609ec5abbbad92bdc91dcac3b8c9/src/index.js#L9-L10

Not mentioning that adding .default is not proper way of supporting a default import declaration. We've discussed this once here - kentcdodds/kcd-scripts#28

Suggested solution:

  1. Remove .default and use exports: 'named' in rollup.config.js or create a separate file for cjs that could wrap the original entry to provide .default
  2. make .resetIdCounter a separate export and not a static property of Downshift class
@kentcdodds kentcdodds added this to the 2.0.0 milestone Mar 20, 2018
@Andarist
Copy link
Collaborator Author

@kentcdodds if you specify which one of those:

  • remove .default and use exports: 'named' in rollup.config.js
  • create a separate file for cjs that could wrap the original entry to provide .default
    would u like to have done, then I can work on this one - although not sure if I manage to do that before the weekend and I'm not sure how fast you'd like to release the new version

@kentcdodds
Copy link
Member

No worries. This change is pretty easy to make so I'll go ahead and do it myself.

I'm going to do:

remove .default and use exports: 'named' in rollup.config.js

Considering the rollup.config.js is already using exports: 'named' 👍

@kentcdodds
Copy link
Member

Fixed!

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

2 participants