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

removing module export from package.json #9

Merged
merged 1 commit into from
May 5, 2017

Conversation

alexreardon
Copy link
Owner

Currently the module export in package.json includes flow code. This is probably not expected. Given that memoize-one has no dependencies there seems to be little utility at this stage in having a module export.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@codecov-io
Copy link

codecov-io commented May 5, 2017

Codecov Report

Merging #9 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #9   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          15     15           
=====================================
  Hits           15     15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc4d5a4...dbd9f1f. Read the comment docs.

@alexreardon
Copy link
Owner Author

I have been thinking about whether this should be a major or minor version bump. This one is tricky. I have gone back and forth. Module loaders should automatically look for the 'main' export if it cannot find a module. However, if you had a setup that you explicitly requested the module export then their code would break as that export no longer exists.

@alexreardon
Copy link
Owner Author

From SemVer.org:

Software using Semantic Versioning MUST declare a public API. This API could be declared in the code itself or exist strictly in documentation. However it is done, it should be precise and comprehensive.

I am not 100% sure if module in the package.json is considered 'public api'

@alexreardon
Copy link
Owner Author

Given that some people might be explicitly relying on module this change would break them. To be super safe I will make this a major version bump

@alexreardon alexreardon merged commit 192ba33 into master May 5, 2017
@alexreardon alexreardon deleted the removing-package-module branch May 5, 2017 06:11
@leandrowd
Copy link

@alexreardon, IMHO, I think you don't need to do this! Consumers can decide whether they want es6 or not. If the answer is NO es6 libraries, the right approach would be to remove "module" from webpack config. If the answer is YES, allow a few directories inside node_modules to go through babel. cc @marcodejongh

@alexreardon
Copy link
Owner Author

@leandrowd it might be unexpected that the es6 module contains flow types which is why I am being really simple and just removing it. In this case there is really no advantage in having module as this library has no dependencies. I see the main use case for module being dependency tree shaking - which is not required for this library

@leandrowd
Copy link

Fair enough @alexreardon.

@alexreardon
Copy link
Owner Author

Users will still have the correct flow types because of the copying of index.flow.js into lib 👍

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.

None yet

3 participants