Skip to content

Use uuid for action IDs instead of Math.random (fixes #1109)#1347

Merged
shilman merged 3 commits intostorybookjs:masterfrom
marcfallows:marc-actions-uuid
Jun 26, 2017
Merged

Use uuid for action IDs instead of Math.random (fixes #1109)#1347
shilman merged 3 commits intostorybookjs:masterfrom
marcfallows:marc-actions-uuid

Conversation

@marcfallows
Copy link
Copy Markdown
Contributor

@marcfallows marcfallows commented Jun 23, 2017

Issue: You can see the issue here: #1109

What I did

I replaced the randomly generated ID with a uuid.

How to test

I included a test, so view/run the unit test included. Or, run storybook for the actions package and you'll see that IDs are now uuids.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 23, 2017

Codecov Report

Merging #1347 into master will increase coverage by 0.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1347      +/-   ##
==========================================
+ Coverage   13.75%   14.12%   +0.36%     
==========================================
  Files         202      201       -1     
  Lines        4623     4609      -14     
  Branches      494      499       +5     
==========================================
+ Hits          636      651      +15     
+ Misses       3556     3524      -32     
- Partials      431      434       +3
Impacted Files Coverage Δ
addons/actions/src/preview.js 50% <100%> (+50%) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/left_panel.js 20% <0%> (ø) ⬆️
app/react/src/server/iframe.html.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/init_panels.js 25% <0%> (ø) ⬆️
app/react/src/client/preview/render.js 0% <0%> (ø) ⬆️
.../src/manager/containers/CommentsPanel/dataStore.js 34.78% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (ø) ⬆️
addons/info/src/components/markdown/htags.js 0% <0%> (ø) ⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 152630b...886dfbf. Read the comment docs.

Copy link
Copy Markdown
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@marcfallows
Copy link
Copy Markdown
Contributor Author

Sweet. Looks like I can't merge, I guess that happens by you guys?

@shilman shilman merged commit be77100 into storybookjs:master Jun 26, 2017
@shilman shilman added the bug label Jun 26, 2017
@ghost ghost mentioned this pull request Apr 2, 2018
ctavan added a commit to ctavan/storybook that referenced this pull request Jul 12, 2019
This uses a purely random v4 UUID instead of a time-based v1 UUID for
the action IDs (which were introduced in storybookjs#1347).

v1 UUID are based on current time and the hardware MAC address of the
machine where they are being generated (although the implementation in
the npm uuid module uses generates a random fake MAC address). As such
they have much more complex semantics than v4 UUIDs which are simply
randomly generated.

Unless there's a specific requirement for the special semantics of v1
UUIDs it is simpler and less error prone to simply go for v4 UUIDs
whenever just a unique identifier is needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants