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

Specify all modules at main of the library #327

Closed
wants to merge 2 commits into from
Closed

Specify all modules at main of the library #327

wants to merge 2 commits into from

Conversation

seia-soto
Copy link

@seia-soto seia-soto commented Sep 21, 2019

I think the library needs to support multiple versions of UUID at one time without requiring more than 2 times. I am using multiple versions at one time, and now there are more lines in my code. It is not a big deal for me. However, I thought if I can do like below(code), it is really helpful.

const uuid = require('uuid')

const v1Result = uuid.v1(); // to use v1
uuid.v5('main', v1Result); // use v5 right after v1

According to #154, the versions were separated into submodules and the structure of the library was changed. The only change I want is specifying all submodules at the main(index.js). From structure, I think there are no changes to use this library.

@broofa
Copy link
Member

broofa commented Sep 25, 2019

80+% of [opensource] projects using this library use v4 and nothing else. Adding dependencies to the top-level module increases the footprint of this module for those users by 6-8X. Unless/until packagers are able to effectively "tree shake" the unused code out of bundles (which, to my knowledge, they do not currently do), I would suggest we not make this change.

@ctavan may have an opinion on this, based on his recent ESM work.

@ctavan
Copy link
Member

ctavan commented Sep 25, 2019

@broofa I believe once we land an ESM variant of this library it will be safe to export all versions from the top-level, at least my experiments with webpack indicated that: https://github.com/kelektiv/node-uuid/tree/a66f7935eb060eb963b6dc21ab90f015cbb078dc/examples/browser-webpack/dist

We could then also include all versions in the CommonJS-build assuming this build will mainly target Node.js users but I think that is a separate discussion.

Until we have a viable alternative for the Web platform I agree with you that we should not further in crease the footprint of the CommonJS-module variant of this module.

@broofa
Copy link
Member

broofa commented Sep 26, 2019

Yeah, that's pretty much my understanding - that tree-shaking is feasible with ESM modules, but isn't practical for CommonJS codebases.

@seia-soto
Copy link
Author

I see why there aren't all versions at the main entry. I am sorry to do this work without considering other uses because I am not good at front-end technologies such as webpack. I can't talk pretty enough about this subject but now understood why. Thanks!

@seia-soto seia-soto closed this Sep 27, 2019
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.

3 participants