-
Notifications
You must be signed in to change notification settings - Fork 12
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
adding a global for unique-id is mega gross #43
Comments
Not pretty, indeed. But it's done this way precisely because of the scenario you mention. Without it, we can't ensure uniqueness of incrementing IDs if you somehow end up with two instances/versions of this module. Cheap, global uniqueness for throw-away ids is the primary usecase here. |
You missed my point. You're not ensuring uniqueness. Package A requires amp-unique-id, starts using it, the counter increments. Later it uses Package B which has a dep of a dep on amp-unique-id, which is a different module than the one Package A is using, it loads it up, it paves the counter, the ids are now zeroed, the Package A continues to use the method now with its ids reset (yikes). I think Underscore's approach is OK here. It's unique for the package that depends on it. While different instances of a module are possible its not as likely that 1 package depends on two different instances. |
Oh shoot, it does zero it out, thanks. I mistook this issue as you just saying it was ugly (as it is) but the fact that it zeros it out is a bug. It being unique for the module that requires it is good, but say you're using two different view modules that generate instances in the client and you're generating DOM ids. It's uniqueness isn't actually guaranteed without the global. Seems quite unlikely, but... is that reason enough to avoid it? |
Ya, dumping to a global is something to be avoided. I imagine if it was an issue Underscore or Lo-Dash would have picked it up and adjusted. Have you looked at other uid implementations. I have a feeling the one that Underscore uses isn't what you're looking for. |
…ngoing but wanted to fix bug referenced in #43
@AmpersandJS/core-team @AmpersandJS/community-leaders thoughts on this? |
OT: Is it possible to make core-team & community-leaders public? |
@jdalton i don't see any way to change it. Why does it matter? |
It'd be rad if Ampersand core, & relevant teams that make decisions in core, were public. Rock! That covers core-team. |
we should add the other group there too. |
If there's nested deps, it happens in node land. This could pave over another instances unique id counter.
The text was updated successfully, but these errors were encountered: