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

Build reproducability when using unique-id() #3347

Open
TLATER opened this issue Jun 24, 2022 · 3 comments
Open

Build reproducability when using unique-id() #3347

TLATER opened this issue Jun 24, 2022 · 3 comments
Labels
enhancement New feature or request planned We would like to add this feature at some point

Comments

@TLATER
Copy link

TLATER commented Jun 24, 2022

Hi, apologies in advance if this should actually be in https://github.com/sass/sass-spec, I'm not really sure where it belongs since the spec just isn't very explicit about the requirements here.

I've been using string.unique-id() as a handy shortcut to make sure my CSS animation keyframe names don't collide with anything. This unfortunately results in reproducibility issues because the randomizer doesn't use a set seed for this.

Is there anything at all that could be done about this? Something like

diff --git a/lib/src/functions/string.dart b/lib/src/functions/string.dart
index 04a441e9..91da035b 100644
--- a/lib/src/functions/string.dart
+++ b/lib/src/functions/string.dart
@@ -14,7 +14,9 @@ import '../utils.dart';
 import '../value.dart';
 
 /// The random number generator for unique IDs.
-final _random = math.Random();
+//
+// By setting the seed, we ensure build-to-build reproducibility.
+final _random = math.Random(1234);
 
 // We use base-36 so we can use the (26-character) alphabet and all digits.
 var _previousUniqueId = _random.nextInt(math.pow(36, 6) as int);

seems attractive, but it would mean that other users of sass would generate the same IDs in their build artifacts, so this could cause collisions again. Any use of math.random() has the same issues as well.

Some way of setting a seed for the randomizer would be very appreciated, but given that this will probably affect all my build dependencies as well it might take a bit more design work than just another library function, especially given some users probably do want random values on each rebuild. A CLI flag would probably help, but might not be sufficient if multiple sass invocations happen (due to, say, distributed builds).

I'd also appreciate a link to the place where the documentation from the website lives - I can't seem to find it, but would like to add a note about this to the doc comments of the affected functions, at least, to make sure users are aware of this limitation.

@sno2
Copy link

sno2 commented Jun 24, 2022

IMO it'd be better to instead have the ability to pass a seed to string.unique-id(1234) to allow everyone to decide the proper solution in their own case. You could then just set a variable to your seed in a given variable to make them all use the same id and the compiler could use a hash map to cache all of these random instances.

@TLATER
Copy link
Author

TLATER commented Jun 24, 2022

Yep, that was what I meant by:

Some way of setting a seed for the randomizer would be very appreciated

My problem is that if any dependency I'm using uses string.unique-id() anywhere then I have no control over the seed they are setting, and therefore cannot resolve the build reproducibility problems without vendoring/patching those dependencies (assuming that most people will not bother to set this, because the problem is historically unnoticed by the community, and build reproducibility issues are largely ignored by the software community anyway).

If there was a meta.seed(1234) that can be called before anything is imported and sets the seed for all random functions for the duration of the build, I think that would be sufficient to solve this at least for single build invocations.

@nex3 nex3 transferred this issue from sass/dart-sass Jun 27, 2022
@nex3 nex3 added enhancement New feature or request planned We would like to add this feature at some point labels Jun 27, 2022
@nex3
Copy link
Contributor

nex3 commented Jun 27, 2022

I think it makes sense to add support for seeding the randomness Sass exposes, but I think it makes more sense to do it as a configuration option than as a feature of the language itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request planned We would like to add this feature at some point
Projects
None yet
Development

No branches or pull requests

3 participants