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

Add export const uuid = v4 #540

Closed
syabro opened this issue Dec 2, 2020 · 17 comments
Closed

Add export const uuid = v4 #540

syabro opened this issue Dec 2, 2020 · 17 comments
Labels

Comments

@syabro
Copy link

syabro commented Dec 2, 2020

Is your feature request related to a problem? Please describe.

For all libraries I use I don't have to write imports manually in WebStorm. All I have to do just write code like const x = doSomething<ALT + Enter> and it shows me hint Add "import doSomething from XXX'?

With export const v4 I have to write manually import as in manual import { v4 as uuidv4 } from 'uuid';. It's frustrating.

Describe the solution you'd like

Change v4() name or add one more export as export const uuid4. So after import it would be very clear what is it about and why.

Describe alternatives you've considered

  • Writing manually :(
  • Adding somewhere utils/uuid.ts with
export { v4 as uuid4} from 'uuid'
@broofa
Copy link
Member

broofa commented Dec 3, 2020

Type const x = v4<ALT + Enter> instead?

@syabro
Copy link
Author

syabro commented Dec 3, 2020

@broofa then a developer would have hard to remember what function with v4 name does. And the name doesn't make any sense unless the developer checks the import. So it makes code harder to read and support.
Also since your recommendation in readme is import v4 as uuid you kinda realize it too.

So why not implement naming that would be more developer-friendly by default?

@ctavan
Copy link
Member

ctavan commented Dec 3, 2020

Thank you for raising this @syabro

I do have to admit that personally I'm also not super happy with the current named exports. They have mostly historic reasons:

  1. When this library was initially released in 2011 for Node.js there was no notion of named exports and not even object destructuring, and in addition this library only supported v4 UUIDs, so the API surface was:

    var uuid = require('uuid');
    uuid();
  2. Then at some point additional algorithms were implemented and the default export merely became an alias for uuid.v4(). IIRC that was still before Node.js 6.0 so object destructuring assignments like const { v4 } = require('uuid') still weren't really a thing:

    var uuid = require('uuid');
    uuid.v4();
    uuid.v1();
    // etc…
  3. Then uuid started to be used in browsers as well and treeshaking became an issue, hence we started encouraging deep imports. But since these deep imported files still only exported a default export that local import names weren't really an issue either:

    var uuidv4 = require('uuid/v4');
    uuidv4();
  4. So only with the migration to ECMAScript Modules (ESM) and named exports the import names became somewhat predetermined.

    import { v4 } from 'uuid';
    v4();

The reason we kept the v4, v1 etc. names when moving to ESM was the idea of keeping the API change as small as possible.

I do admit though that in the ESM world we live in by now requiring aliasing when importing one of the algorithms is really not ideal and at least I didn't really consider that aspect enough when doing the migration to ESM.

All that said I believe this might be a good chance to get rid of the confusing v1, v4, … naming entirely. After all it seems to be causing more harm than good, see discussions in tc39/proposal-uuid#3 and this analysis.

So instead of sticking to these names I suggest to export the algorithms with more telling names, like discussed in tc39/proposal-uuid#3 (comment). Something like:

v1() -> timeUUID()
v4() -> randomUUID()
v3() -> md5UUID() // (or nameUUID()?)
v5() -> sha1UUID() // (or nameUUID()?)

An alternative would obviously be to export uuidv4() instead of v4(), this naming would be similar to what Python does

Thoughts?

@syabro
Copy link
Author

syabro commented Dec 3, 2020

@ctavan thanks for clarifying.

randomUUId() / uuidv4() would be great since it's after reading it's obvious what function does.

PS As soon it's not default export I will be happy to see it :)

@broofa
Copy link
Member

broofa commented Dec 3, 2020

"There are only two hard things in Computer Science: cache invalidation and naming things"

I suspect this is exactly what Phil Karlton had in mind with the latter part of that quote. There's no clearly "right" naming scheme here. Developers are going to have to remember what the name is regardless of what we choose. The current scheme at least has the advantage of not having to remember how UUID/uuid/Uuid/UUId is capitalized. 😝

I don't see the "more harm than good" you mention of the current scheme.... at least not to an extent that warrants (again) breaking the API for M's of users. My vote? Stick with the current scheme until there's broader consensus on what the names should be. I.e. Wait until the standard uuid work settles out, and then go with whatever API scheme is decided there.

@LinusU
Copy link
Member

LinusU commented Dec 3, 2020

"There are only two hard things in Computer Science: cache invalidation and naming things"

I've always known that one as:

"There are only two hard things in Computer Science: cache invalidation, naming things, and off by one errors"

😄

Wait until the standard uuid work settles out, and then go with whatever API scheme is decided there.

I'm leaning towards this also, even if I also feel the frustration every time I import the function. If we change to something like timeUUID() now, and then TC39 settles on another name, we'll probably want to change again.

@ShadowDancer
Copy link

This is great PR, for god sake why I would always name function before importing it. Just name it uuidv4. uuidStringify and so on.

@gizm0bill
Copy link

Angular compilation warning: application.service.ts depends on 'uuid'. CommonJS or AMD dependencies can cause optimization bailouts. For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

@TrySound
Copy link
Member

@gizm0bill This is not related issue. Please create new one if there is a bug.

@gizm0bill
Copy link

@gizm0bill This is not related issue. Please create new one if there is a bug.

Actually it is...

@TrySound
Copy link
Member

@gizm0bill From the warning it looks like you use some very old version without es modules support.

@sholladay
Copy link

sholladay commented Apr 9, 2021

FYI, you can just do this:

import * as uuid from 'uuid';

uuid.v4()

I quite like this syntax, actually. I think it should be recommended in the README. The main caveat is that, last I checked, treeshaking generally doesn't work when importing the whole namespace. I don't personally feel that matters too much here, though.

@TrySound
Copy link
Member

TrySound commented Apr 9, 2021

@sholladay rollup and webpack 5 does not have the problem with namespaces. They are expanded into named imports unless used as value.

@syabro
Copy link
Author

syabro commented Apr 10, 2021

@sholladay writing it manually is the same as import {v4 as uuidv4} from 'uuid'

@sholladay
Copy link

Sort of, @syabro. It's the same in that I showed an alternative way to use the v4 function, via namespace import instead of as a named import. It's the same function, but accessed a different way. In my opinion, the namespace import is much less ugly.

It could be simplified further by attaching the functions as properties of the default export object. Then you'd be able to do this:

import uuid from 'uuid';

uuid.v4();

That would be the cleanest syntax.

@github-actions
Copy link

Marking as stale due to 90 days with no activity.

@github-actions github-actions bot added the stale label Aug 13, 2022
@github-actions
Copy link

Closing issue due to 30 days since being marked as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants