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

First working version with code extracted from Node.js 15 #1

Merged
merged 30 commits into from
Mar 23, 2021

Conversation

piranna
Copy link
Member

@piranna piranna commented Feb 8, 2021

It also has tests, it's compatible with both Node.js and browser, and have been removed Node.js specific code, but lacks of documentation.

@TrySound
Copy link
Member

TrySound commented Feb 8, 2021

IMO this should be importable module in the first place and then maybe polyfill.

@piranna
Copy link
Member Author

piranna commented Feb 8, 2021

IMO this should be importable module in the first place and then maybe polyfill.

Can you explain a bit more? randomUUID.js file is requirable.

@TrySound
Copy link
Member

TrySound commented Feb 8, 2021

index.js is polyfill. Patching builtin objects should not be default.

@piranna
Copy link
Member Author

piranna commented Feb 8, 2021

I can change that, but purposse of this project has been to create a polyfill since begining, that's the reason of the browser field. But yes, polyfile filename could be renamed to node.js, for example.

@TrySound
Copy link
Member

TrySound commented Feb 8, 2021

Polyfill for node + implementation for browser. The api is not part of any spec and is node specific.

@piranna
Copy link
Member Author

piranna commented Feb 8, 2021

The api is not part of any spec

It is :-)

@TrySound
Copy link
Member

TrySound commented Feb 8, 2021

Oh, cool. Thanks! I missed this.

Copy link
Member

@broofa broofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Polyfill for node + implementation for browser. The api is not part of any spec and is node specific.

Echoing this sentiment. This is a polyfill for Node's implementation. While that is the only implementation currently in the wild, I expect that will change fairly quickly as the WICG work progresses. Specifically:

  • disableEntropyCache is unlikely to be part of the spec
  • If/when type assertions are specified, I would expect them to be TypeErrors, not the adhoc error classes seen here.

I'm okay moving forward with this but once there's a formal standard, I would assume this module switches to whatever that is (as a breaking change), with Node either leading or following the way on that I guess.

Also, there should be browser tests as well. Maybe take a page out of the uuid module's approach? (Kind of a nuisance to do, though. @ctavan: thoughts?)

README.md Outdated Show resolved Hide resolved
randomUUID.js Show resolved Hide resolved
@piranna
Copy link
Member Author

piranna commented Feb 27, 2021

Sorry for the delay. I've reviewed and fixed your concerns, and made sure tests pass on both Node.js (down to v12) and JsDOM environments, with almost 100% tests coverage. There's an issue with jest that prevents to resolve the browser field, but it's just only a line of code, so we can control it.

Thoughts? Comments?

@piranna
Copy link
Member Author

piranna commented Mar 9, 2021

Any update on this? How can we move it forward?

Copy link
Member

@broofa broofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nit-picky changes here, but nothing critical. Approving to allow this to move forward.

Note: A few things I'd like to see added in the near future, if possible:

  • code linting (preferably using uuid. I.e. eslint + uuid's .eslintrc.json)
  • markdown linting (preferably consistent with uuid. I.e. prettier + uuid's prettier.config.js)
  • CI (preferably GH Action)

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
__tests__/randomUUID.js Outdated Show resolved Hide resolved
__tests__/randomUUID.js Outdated Show resolved Hide resolved
__tests__/randomUUID.js Outdated Show resolved Hide resolved
@broofa
Copy link
Member

broofa commented Mar 10, 2021

@ctavan @TrySound, You guys good with this? I know it's not as far along as uuid in terms of linting and CI testing but we can backfill those things as needed.

Also, I suggest we invite @piranna to uuidjs org once this lands.

piranna and others added 3 commits March 14, 2021 13:23
Co-authored-by: Robert Kieffer <[email protected]>
Co-authored-by: Robert Kieffer <[email protected]>
Co-authored-by: Robert Kieffer <[email protected]>
@piranna
Copy link
Member Author

piranna commented Mar 14, 2021

Added linting fixes based on uuidjs/uuid project config files. Also applied all suggestions of @broofa.

Copy link
Member

@broofa broofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks likeeslint-plugin-import, eslint-plugin-node,eslint-plugin-promise, and prettier all need to be added as devDependencies.

(Couldn't get npm test to work until I installed these.)

@broofa
Copy link
Member

broofa commented Mar 14, 2021

Oh, we should probably also go ahead and add "sideEffects": true to package.json. From https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

A "side effect" is defined as code that performs a special behavior when imported, other than exposing one or more exports. An example of this are polyfills, which affect the global scope and usually do not provide an export.

This is mostly relevent for ESM imports, which aren't supported here yet, but there's no harm in being explicit about this now.

@piranna
Copy link
Member Author

piranna commented Mar 14, 2021

It looks likeeslint-plugin-import, eslint-plugin-node,eslint-plugin-promise, and prettier all need to be added as devDependencies.

(Couldn't get npm test to work until I installed these.)

In my machine I've not needed to add them...

Ubuntu Mate 20.10
Node.js v15.11.0
npm 7.6.0

@piranna
Copy link
Member Author

piranna commented Mar 14, 2021

we should probably also go ahead and add "sideEffects": true to package.json

Seems to be really interesting :-)

@broofa
Copy link
Member

broofa commented Mar 14, 2021

In my machine I've not needed to add them...

Do you have them installed globally?

@piranna
Copy link
Member Author

piranna commented Mar 14, 2021

Do you have them installed globally?

No, my globals are only these ones:

[piranna@PRO2:~/github/randomUUID]
 (main) > npm ls -g
/usr/lib
├── [email protected]
├── [email protected]
├── [email protected]
└── [email protected]

@broofa
Copy link
Member

broofa commented Mar 15, 2021

No, my globals are only these ones:

Here's my recipe for recreating the missing modules issue...

$ node --version
v14.15.4

$ npm --version
6.14.10

$ pwd
/Users/kieffer/repos/randomUUID

$ git status
On branch piranna-main
nothing to commit, working tree clean

$ rm -fr ./node_modules
...
$ npm install
...
$ npm test
...
Oops! Something went wrong! :(

ESLint: 7.22.0

ESLint couldn't find the plugin "eslint-plugin-import".

etc...

@piranna
Copy link
Member Author

piranna commented Mar 15, 2021

I think npm v7 is installing missing peerDependencies... could that be the reason?

@piranna
Copy link
Member Author

piranna commented Mar 15, 2021

Ok, I've checked it with Node.js v14.16.0 and npm v6.14.11 and I'm seeing the same problem as you, so seems it's the thing about npm 7 installing peerDependencies. I'm going to add the missing dependencies.

@piranna
Copy link
Member Author

piranna commented Mar 15, 2021

I've added the missing dependencies.

@broofa broofa merged commit 8b7d247 into uuidjs:main Mar 23, 2021
@broofa
Copy link
Member

broofa commented Mar 23, 2021

My apologies for the delay. I've gone ahead and landed this since, as previously noted, I think we're at a place where we can push this out and deal with issues as/how they get reported.

I've also taken the following actions (@ctavan, take note):

  • Invited @piranna to the uuidjs org as a "member"
  • npm owner add piranna (... but piranna is not appearing yet under npm owner ls. Assuming that's because he needs to accept the invitation?)
  • Added randomUUID to the uuid team (w/in this org)
  • Enabled the new github "discussions" feature for the uuid team

@piranna: I'll leave it to you to do the npm publish honors.

@broofa
Copy link
Member

broofa commented Mar 23, 2021

Also, thanks for your work and, most of all, your patience. :-)

image

@piranna
Copy link
Member Author

piranna commented Mar 23, 2021

My apologies for the delay.

No worries :-)

  • Invited @piranna to the uuidjs org as a "member"

Thank you so much! :-D

  • npm owner add piranna (... but piranna is not appearing yet under npm owner ls. Assuming that's because he needs to accept the invitation?)

I've not received any invitation, just only the one for Github org... Maybe there's some missing step? On https://www.npmjs.com/org/uuid it shows nothing too, but I appear in https://www.npmjs.com/package/randomuuid.

@piranna: I'll leave it to you to do the npm publish honors.

403 Forbidden - PUT https://registry.npmjs.org/randomuuid - You cannot publish over the previously published versions: 0.1.0-alpha.0.

What version should I use? 0.1.0?

@piranna
Copy link
Member Author

piranna commented Mar 23, 2021

Also, thanks for your work and, most of all, your patience. :-)

You are welcome, I'm glad to help here :-)

@broofa
Copy link
Member

broofa commented Mar 24, 2021

403 Forbidden - PUT https://registry.npmjs.org/randomuuid - You cannot publish over the previously published versions: 0.1.0-alpha.0.

What version should I use? 0.1.0?

I'd go with 1.0.0. Minor-bump it once the ESM support gets rolled in.

Edit: npm owner ls is showing you as an owner now, so I think we're good there. Probably just a caching delay of some sort.

@piranna
Copy link
Member Author

piranna commented Mar 24, 2021

And... published :-)

@ctavan
Copy link
Member

ctavan commented Mar 25, 2021

Woah, amazing work @piranna (and @broofa @TrySound)!

I'm sorry for the radio silence from my end. Started a new job recently so my priorities have shifted a bit. I'm confident that I'll be able to contribute again once I've onboarded :).

@piranna
Copy link
Member Author

piranna commented Mar 26, 2021

You are welcome :-)

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.

4 participants