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

Support for relative dependencies for define(). #2

Closed
wants to merge 1 commit into from

Conversation

nevir
Copy link
Owner

@nevir nevir commented Feb 26, 2015

Note that this implementation is a little naive; it completely ignores
the id given to define, and always assumes that URLs will be relative
to the script's document. That will not work for vulcanized bundles.
Separate PR for that.

Note that this implementation is a little naive; it completely ignores
the id given to define, and always assumes that URLs will be relative
to the script's document. That will not work for vulcanized bundles.
Separate PR for that.
@nevir nevir force-pushed the 0.8-amd-implicitly-relative branch from 1815237 to 7464430 Compare February 26, 2015 01:38
@nevir
Copy link
Owner Author

nevir commented Feb 26, 2015

define.withImports is slightly dangerous (if someone else monkey patches define). Maybe we want it as a real global

@justinfagnani
Copy link

I rather like define.withImports do we have any evidence that other AMP systems stomp on define?

id = _inferModuleId();
id = inferredId;
}
if (id.indexOf('\\') !== -1) {

Choose a reason for hiding this comment

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

document that \ is not allowed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'd like to actually support it in the name of browser compatibility (AFAIK everyone but Firefox rewrites \ into / anyway). I'll stick a TODO on there

@justinfagnani
Copy link

flippin' awesome: LGTM

@nevir
Copy link
Owner Author

nevir commented Aug 12, 2022

Killing off some of my really old PRs

@nevir nevir closed this Aug 12, 2022
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

Successfully merging this pull request may close these issues.

2 participants