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

fix(cli/import_map): Don't statically error on dynamic unmapped bare specifiers #10618

Merged
merged 5 commits into from
May 31, 2021

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented May 12, 2021

Fixes #10168.
Fixes #10615.
Fixes #10616.

cli/import_map.rs Outdated Show resolved Hide resolved
@nayeemrmn nayeemrmn marked this pull request as draft May 12, 2021 19:33
@nayeemrmn nayeemrmn force-pushed the import-map-unmapped-bare-specifier branch from 7042ee4 to 44e9598 Compare May 12, 2021 23:39
@nayeemrmn nayeemrmn changed the title fix(cli/import_map): Don't error on unmapped bare specifiers fix(cli/import_map): Don't statically error on unmapped bare specifiers May 12, 2021
@nayeemrmn nayeemrmn marked this pull request as ready for review May 12, 2021 23:40
@nayeemrmn nayeemrmn force-pushed the import-map-unmapped-bare-specifier branch from e156127 to 5514e1e Compare May 19, 2021 00:34
@bartlomieju
Copy link
Member

bartlomieju commented May 19, 2021

I'm not entirely sure about this change - it seems to start deviating from the spec algorithm, but on the other hands none of the WPT fail so I guess it's okay.

@nayeemrmn
Copy link
Collaborator Author

Which part deviates from the spec algorithm? The two important changes are:

  1. Structured import map errors (https://github.com/denoland/deno/pull/10618/files#diff-fba55433053f0ea5eef635f4d576f13beea8b8091c1d692f3513e46e4715e0d3L17-R20).
  2. Solidifying Module::parse(), which doesn't error on ModuleResolutionError::ImportPrefixMissing, to also not error on its equivalent ImportMapError::UnmappedBareSpecifier (https://github.com/denoland/deno/pull/10618/files#diff-8439fae0dccaa5fecd30e137ceb78680f3b7884c1ae74a5f7c649f5273abf3e2L384-R391). This was the cause of the bug. I don't think it correlates with any spec.

All other changes are 1-to-1 simplifications e.g. the ImportMap::resolve() never returns None, so the Option is removed from its return type.

@bartlomieju
Copy link
Member

Which part deviates from the spec algorithm? The two important changes are:

  1. Structured import map errors (https://github.com/denoland/deno/pull/10618/files#diff-fba55433053f0ea5eef635f4d576f13beea8b8091c1d692f3513e46e4715e0d3L17-R20).
  2. Solidifying Module::parse(), which doesn't error on ModuleResolutionError::ImportPrefixMissing, to also not error on its equivalent ImportMapError::UnmappedBareSpecifier (https://github.com/denoland/deno/pull/10618/files#diff-8439fae0dccaa5fecd30e137ceb78680f3b7884c1ae74a5f7c649f5273abf3e2L384-R391). This was the cause of the bug. I don't think it correlates with any spec.

All other changes are 1-to-1 simplifications e.g. the ImportMap::resolve() never returns None, so the Option is removed from its return type.

@nayeemrmn I see, somehow I thought spec required to return null/None when the resolution fails, but it seems you're just unifying the behavior.

LGTM then, I will let @kitsonk take a look before landing

@bartlomieju bartlomieju requested a review from kitsonk May 20, 2021 13:05
@nayeemrmn nayeemrmn force-pushed the import-map-unmapped-bare-specifier branch from 5514e1e to d274a90 Compare May 29, 2021 06:35
@kitsonk
Copy link
Contributor

kitsonk commented May 30, 2021

Sorry, for some reason this was missing from my todo list... I will look at first thing tomorrow my time.

@nayeemrmn nayeemrmn force-pushed the import-map-unmapped-bare-specifier branch from d274a90 to 488c003 Compare May 30, 2021 13:18
@nayeemrmn nayeemrmn changed the title fix(cli/import_map): Don't statically error on unmapped bare specifiers fix(cli/import_map): Don't statically error on dynamic unmapped bare specifiers May 30, 2021
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants