Skip to content

do not treat modules with '!' in names any specially #5759

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

Merged
merged 2 commits into from
Nov 23, 2015
Merged

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Nov 23, 2015

fixes #5729

@mhegazy
Copy link
Contributor

mhegazy commented Nov 23, 2015

👍

1 similar comment
@RyanCavanaugh
Copy link
Member

👍

@weswigham
Copy link
Member

Just to confirm, this was originally in here to support systemjs loader plugins, no?

vladima added a commit that referenced this pull request Nov 23, 2015
do not treat modules with '!' in names any specially
@vladima vladima merged commit fb76dc9 into master Nov 23, 2015
@vladima vladima deleted the bangInModuleNames branch November 23, 2015 22:08
@mhegazy
Copy link
Contributor

mhegazy commented Nov 24, 2015

@weswigham i really do not know why this was there :) the issue is that SystemJs and requireJS have a different way to represent loaders. so what we have done in the past, is we do nothing :) we just treat it like a module name, obviously a file lookup will fail, but then we will look for an ambient external module with that name, and if you defined one, things should just work.

@kitsonk
Copy link
Contributor

kitsonk commented Nov 24, 2015

and it has fixed the issue...

Technically, it isn't RequireJS, it is the whole of AMD ;-)

I can understand why it might have seemed like a good idea, but the only approach I can envision is that the module resolution logic narrows to the bang instead of widening to the bang and/or if the module contains a bang, it looks at the ambient module space first before attempt to resolve to the file.

What was happening is that any MID like 'foo/bar!something' was trying to resolve to foo/bar and not considering anything else. I don't think ideally that would have worked for SystemJS either, since the stuff after the bang affects the format of the return (where the reverse is true in AMD).

If I had a mid of some/file.txt!text in SystemJS, I would ideally have to interrogate the SystemJS config to figure out what text did, but in theory, I could match that to some sort of ambient declaration like this:

declare module 'text' {
  export default let text: string;
}

I might benefit from validating that I can actually resolve some/file.txt but really from TypeScript purposes, all I need to know is the ambient declaration.

For AMD it is reversed though, while we have just gotten used to declaring ambient modules for each different plugin, it would be nice to actually declare something like this:

declare module 'dojo/text' {
  export default let text: string;
}

And then have anything like 'dojo/text!some/file.txt' work, but the problem specifically I was running into though was that would have worked before this fix, but then when I had a module that returned different things based on the information past the bang, it would never find the ambient declaration.

If I really had my dreams filled, there would be some sort of globbing support for mids and ambient modules. Maybe I should open an issue for that.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 24, 2015

@kitsonk thanks for the details. question, would it possible that foo\bar!text and baz!text would have a different shapes? or is it guaranteed that they have the same shape?
would it be sufficient to for module names of the form [resource]![extension] to look up 1. ambient external module [resource]![extension] (for back compat purposes) and if not found, lookup 2. ambient external module [extension]?

@kitsonk
Copy link
Contributor

kitsonk commented Nov 25, 2015

The shape can vary. I opened #5787 to put forward a suggestion around this.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with AMD Plugin syntax
6 participants