prantlf / date-fns-timezone

Parsing and formatting date strings using IANA time zones for date-fns.
MIT License
137 stars 18 forks source link

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

Open Debananda opened 5 years ago

Debananda commented 5 years ago

made the methods individually exportable to reduce size

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 23


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

💛 - Coveralls
jasongerbes commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

danazkari commented 5 years ago

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 commented 5 years ago

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!