Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
lib: make sure globals can be loaded after globalThis is sealed #46831
base: main
Are you sure you want to change the base?
lib: make sure globals can be loaded after globalThis is sealed #46831
Changes from 2 commits
7c6fc71
8392842
fffbcbd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't think having only the keys is an improvement for debugging the failures? At least it helped my figuring out what's going on when I could just
console.log()
the failures. Sometimes I think there is a tendency in our tests that made the tests too simple and DRY so it makes the tests harder to debug, but I'd personally prefer debuggability over simplicity...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you post your comment on the right thread? This suggestion is removing a filter (so we run the test on all the keys), it's not adding one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, the comment should be posted to the for-loop below. But I don't think we need to run the test on all keys though, otherwise we run into the undici dispatch symbol, and I don't see that it's Node.js core that should test the symbol works on a frozen globalThis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restricting to string keys is a bit weird, I think it would be better to be able to access all global keys when the global object is sealed, and add to one that would fail to the expected failures set. If that's harder than it's worth, we should add a comment explaining that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'd only care about string keys because it would be weird that contextual lookup on the global stops working once globalThis is sealed. Symbols not so much because one can't do contextual lookup on the global with symbols, and also these symbols aren't necessarily visible to/fixable by Node.js core (like the undici one). But I can add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it deleting the failure (which is undeclared if we apply the suggestion above)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I meant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this version would lead to less debuggable messages - for example if the error is caused by redefining something else on a global in the implementation of another (e.g. the undici symbol for the known failures listed here), the error message would not show which key is causing the trouble, and only shows the symbol which is an implementation detail of that key. So fixing the known failures takes 4 trial-and-errors. Whereas having a failures map for them all and logging the map only takes 1 trail-and-error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of the following then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And also I personally prefer to avoid one-line helpers like
access()
here in the test because they make the test failures harder to read. IMO tests are usually easier to understand if we can just be verbose and write the code being tested directly even if it involves some repetition - as long as it's not too much repetition)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least you can agree that the
success
map is not useful, nor for the test itself, and likely not for debugging either?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is more complicated than the original version, which just adds results on success and error on failure to two maps instead of trying to add or delete anything? At least it'd take me a while to understand what the second version is doing, whereas the original one doesn't take much brain power to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That avoids having to add a comment to skip the no-unused-expressions eslint check. I figure if we have to throw extra code to avoid it, might as well just put them into a map, in case one wants to find out what can actually be loaded (for example, to find out
fetch
can be loaded, but other fetch-related classes can't).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.