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

Values with trailing slashes not in path part #173

Closed
hiroshige-g opened this issue Aug 15, 2019 · 8 comments
Closed

Values with trailing slashes not in path part #173

hiroshige-g opened this issue Aug 15, 2019 · 8 comments
Milestone

Comments

@hiroshige-g
Copy link
Collaborator

Originally from @nyaxt:

For keys ending with slashes, we require values to end with slashes, but any slashes (not necessarily in the path part) are OK. So for example

{
  "imports": {
    "https://foo.com/": "https://nyaxtstep.com/fox?grapes=/",
    "https://bar.com/": "https://nyaxtstep.com/fox#/"
  }
}
  • https://foo.com/bar/baz is mapped to https://nyaxtstep.com/fox?grapes=/bar/baz
  • https://foo.com/bar?baz is mapped to https://nyaxtstep.com/fox?grapes=/bar?baz
  • https://bar.com/baz is mapped to https://nyaxtstep.com/fox#/baz

(The behavior before d5add26 is different though)

I don't have strong opinions here; WDYT? Does this looks good?

@domenic
Copy link
Collaborator

domenic commented Aug 19, 2019

Hmm. This seems OK-ish; the intention of the rule is mostly to catch potential errors. However it seems like it would be pretty easy to add one additional check, that the trailing slash is in the path.

I guess it would become harder to insert such a check after #167, because then the right hand side is no longer necessarily a URL. /cc @bakkot @michaelficarra.

@bakkot
Copy link

bakkot commented Aug 19, 2019

I think it'd still be pretty easy to add this check after #167; URL-based specifiers on the right-hand side already need to be normalized, and there could be an additional check at that point forbidding query parameters or fragments when the left hand side is a package prefix. This would involve adding an additional "isValidPackagePrefixRHS" field to the result of tryURLLikeSpecifierParse in the URL case, to go along with the existing isBuiltin, or something like it, but I don't think it would introduce much complexity.


My opinion on the issue:

The https://nyaxtstep.com/fox?grapes=/ case, in particular, seems like it ought not be forbidden: for example, you might have a single node_resolver.php resource responsible for serving all JavaScript which lives in node_modules, and want to do

{
  "imports": {
    "moment/": "/node_resolver.php?path=moment/"
  }
}

(Of course one could rewrite their server in such a way that the resource ended up in the actual path of the URL, but why should they have to?)

So adding this check, at least for the query parameter case, seems like it would rule out useful cases, and I'm not convinced it rules out any commensurately large class of error. The # case might be worth forbidding, since the part after the # won't even be visible to the server (I assume); on the other hand, it's not obviously desirable to carve out an exception for it, since it makes the model more complicated.

@bakkot
Copy link

bakkot commented Aug 19, 2019

Also, re: the # case, it doesn't seem like it's obviously any more of an error to have a # in a package-prefix URL than any other URL on the RHS. That is, it seems like both of the mappings in the following should be an error, or neither should be:

{
  "imports": {
    "a/": "/a#foo/",
    "b": "/b#bar"
  }
}

@guybedford
Copy link
Collaborator

@bakkot on the last point, there can be value in the definition of:

{
  "imports": {
    "b": "/b#bar"
  }
}

as the #bar effectively causes reinstancing like a sort of cache busting just at the module registry level (without triggering refetch) of the module, such that import '/b' would be a different execution instance of the same module.

@dcleao
Copy link

dcleao commented Sep 11, 2019

In a related issue, I propose a syntax that would allow the excess segments "matching" the / on the LHS to be inserted in a place other than the end of the RHS: #134 (comment).

The addition of this feature would dedramatize the choice of where the excess segments are placed by default.

@domenic
Copy link
Collaborator

domenic commented Sep 23, 2019

It seems like we're OK with the consequences of the current spec, so I'll close this. Happy to reopen if there's something we missed.

@domenic domenic closed this as completed Sep 23, 2019
@domenic domenic added this to the MVP milestone Sep 23, 2019
@hiroshige-g
Copy link
Collaborator Author

#176 reverts the trailing-slash behavior back to pre-#164 (still I'm neutral):

  • https://foo.com/bar/baz is mapped to https://nyaxtstep.com/bar/baz
  • https://foo.com/bar?baz is mapped to https://nyaxtstep.com/bar?baz
  • https://bar.com/baz is mapped to https://nyaxtstep.com/baz
  • moment/foo is mapped to /foo

@TomasHubelbauer
Copy link

TomasHubelbauer commented Jan 19, 2024

As of current, does the spec allow using import maps to route imported modules through a single endpoint passing their path via a fragment or a search param like shown in this snippet by @bakkot?

{
  "imports": {
    "moment/": "/node_resolver.php?path=moment/"
  }
}

I am hoping to find a way to achieve this:

<script type="importmap">
  {
    "imports": {
      "./index.js": "./index.js",
      "./": "./launder.js?/"
    }
  }
</script>
<script type="module" src="index.js"></script>

index.js:

import test from './test.js';

console.log(import.meta.url, test);

launder.js:

console.log(import.meta.url);

export default import.meta.url;

In this scenario, in my preferred case, I would like to see ./test.js resolve to ./launder.js?/test.js and launder.js should then print …/launder.js?/test.js followed by index.js printing …/index.js …/launder.js?/test.js.

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

No branches or pull requests

6 participants