-
Notifications
You must be signed in to change notification settings - Fork 449
Handful of new module registry edits #5142
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
Conversation
9f0546e
to
140fa60
Compare
* Disable new module registry for python workers * Improve normalization of module names from the user bundle, with tests
Ensure that module specifiers cannot escape the bundle base URL using relative path leading dot segments. Ignore leading whitespace in module specifiers. Ensure correct handling of Unicode characters in module specifiers.
140fa60
to
dd00a33
Compare
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.
Thanks for taking on board the feedback and terminology discussion. Depending on how normalizeModuleName
is used it might well be a specifier function after all - basically "module specifier" for any string that is at the very beginning of the pipeline and will yet go through every single step of module resolution rules (including possible package mappings / import mappings), while everything else gets named according to where it is in the pipeline helps a lot in ensuring consistent processing.
Because I saw this used for addSyntheticModule
, etc. which I'd assumed would apply to at least path-like module names, that's the reason I was wondering if it's not a specifier but rather a path-like -input. Good to have the terminology discussion though anyway.
Just one remaining question otherwise great to land.
|
Specifically on the second bullet point... All modules provided by the user bundle must remain subordinate to the bundle root (default
file:///bundle/
), which means if the worker bundle happens to use a name like../foo
, we don't want that module id to befile:///foo
, we still want it to befile:///bundle/foo
. The handling here to tolerant of silliness. If the name in the worker bundle is something like/.././/./..//.////../../foo
, then it'll still be resolved tofile:///bundle/foo
. Leading whitespace,/
s, single-dot, and double dot segments will be removed such that the resolution against the basefile:///bundle/
works correctly.The PR also fixes up handling of the case where the module name is using non-ASCII characters. These will be converted to UTF-8 both in the bundle and in the source and the original handling code was not interpreting those correctly. Tests are added to verify correct handling.