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

all the methods are individually exportable to reducce the package size #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Debananda
Copy link

made the methods individually exportable to reduce size

@coveralls
Copy link

coveralls commented Mar 10, 2019

Pull Request Test Coverage Report for Build 23

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 19: 0.0%
Covered Lines: 47
Relevant Lines: 47

💛 - Coveralls

@jasongerbes
Copy link

My understanding is that the package-lock.json should not be removed.

If some of the package versions in the package-lock are no longer available, you should run npm i and commit the updated package-lock.json file.

@schibrikov
Copy link

It seems like you've just changed named exports to default ones. I don't know any rationale behind such decision. All the methods exported individually without this changes and you can import them directly just like import { formatToTimeZone } from 'date-fns-timezone/dist/formatToTimeZone';.

Also, ignoring lockfiles is absolutely incorrect. They exist for being committed.

@danazkari
Copy link

@sHooKDT I do agree with the ignoring lock files, that's never a good thing 😄 But the rationale behind exporting individual functions is to keep package size small, this allows us to use tree-shaking in order to not import functions we're not using.

Here's how date-fns does it import format from 'date-fns/format'; which only imports that function and instead of including the entire library on my final build, it'll only include that function.

This a very much needed feature in the project I'm working on, can we please fix this PR and merge it? I can work on it if needed.

@devpato
Copy link

devpato commented Jul 3, 2019

@jasongerbes I agree with you. package-lock.json
https://docs.npmjs.com/files/package-lock.json
https://stackoverflow.com/questions/44206782/do-i-commit-the-package-lock-json-file-created-by-npm-5

@danazkari
Copy link

I can create my own PR with a different take on this, to support both ways of exporting.

@danazkari
Copy link

#11
This is my take on how to do this with the least amount of friction, maintaining previous support plus also giving the users of this API the option to do so in a performant way (only including what they require)

@prantlf
Copy link
Owner

prantlf commented Jul 3, 2019

I agree with @jasongerbes, that the package lock should stay, as @devpato referred to the best practices about it. They keep the build of this package stable and avoid security problems, which npm audit fix can solve. (Most modules are in devDependencies.) Other projects can pin different modules versions in their package locks. An additional benefit is in the ability to use npm ci to speed up module installation during often automated builds.

Like @sHooKDT, I wonder, what is the rationale behind using the default exports instead of named imports. Is there are multiple exported items, name exports allow tree-shaking (unreachable code elimination). Wildcard exports are usually frowned upon, because unused exported items cannot be easily detected by the module bundler. named exports make easy to prune the originating module off all code, which is not used not only in any exported function, but also in any exported function, which is not imported by any other module. I do not know, if they analyse all source code to find out, what functions from the imported object are actually accessed. That is the case of the "bundled-interface" modules src/index.js and src/custom/js.

https://news.ycombinator.com/item?id=15765409
https://www.reddit.com/r/javascript/comments/9amkc6/export_default_considered_harmful/
https://blog.neufund.org/why-we-have-banned-default-exports-and-you-should-do-the-same-d51fdc2cf2ad
https://humanwhocodes.com/blog/2019/01/stop-using-default-exports-javascript-module/
https://basarat.gitbooks.io/typescript/docs/tips/defaultIsBad.html

Most JavaScript modules in the src directory export just a single function to keep the sources better maintainable. Bundles src/index.js and src/custom/js make the integrations easier, which prefer concatenated bundles, which still support the tree-shaking. For the single-function-exporting modules, I admit, the named export might not show any objective benefit, which would show performance, size, or coding efficiency difference. I made these module use the named exports nevertheless, because it makes all modules consistent and it actually helped me, when I renamed some before the release, to make sure, that I did not forget anything.

I'm reluctant to agree with switching the default exports, because it is a breaking change on the interface, as long as I do not see an objective or subjective advantage of it.

@danazkari, how do the named exports prevent you from importing just a single function? How do they hinder the performance? Thank you!

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.

7 participants