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

angular2.dev.js contains incorrect SystemJS config #3826

Closed
IgorMinar opened this issue Aug 25, 2015 · 12 comments
Closed

angular2.dev.js contains incorrect SystemJS config #3826

IgorMinar opened this issue Aug 25, 2015 · 12 comments

Comments

@IgorMinar
Copy link
Contributor

At the end of the file we have:

System.config({"paths":{"*":"*.js","angular2/*":"angular2/*"}});

I'm not sure how we ended up with the "*":"*.js" mapping, but we shouldn't be configuring paths not belonging exclusively to Angular.

This is especially confusing as of a SystemJS 0.17 when defaultJSExtension was flipped to false.

@pkozlowski-opensource
Copy link
Member

I agree with @IgorMinar - I don't see any reason for this config after migration to a newer SystemJS. More generally, I don't think we should be in business in providing System config. Going to send Pr to remove this.

@jpsfs
Copy link

jpsfs commented Aug 27, 2015

Just a quick question, using the new version of SystemJS shouldn't all modules be registered with the JS extension?

Example:

Before (system ~0.16)

System.register("angular2/src/di/metadata", ...)

After (system ~0.18)

System.register("angular2/src/di/metadata.js", ...)

@pkozlowski-opensource

@pkozlowski-opensource
Copy link
Member

@jpsfs while it is true that never systemjs moves to URLs as module modifiers it doesn't matter much in a bundled version since everything is pre-loaded and no HTTP / Ajax request should be done at runtime for ng2-specific resources.

We could register things with .js extension but this would be non-trivial (?) with out current setup in the repo where imports are shared between *.ts and *.dart files.

@rkirov
Copy link
Contributor

rkirov commented Aug 27, 2015

I added that line, as the original goal of the bundle was to make getting off the ground as quick and painless as possible. That allowed for demoes to have zero system.config, and jump straight to System.import.

So why not replace it with the 0.18 version defaultJsExtension: true, so that demo users don't feel like they need to learn system.js configuration options. Or my assumption that most demos need defaultJsExtension: true is wrong?

@pkozlowski-opensource
Copy link
Member

@rkirov I see your point about trying to make it easier on users. The pb I can see with this approach is that we are enforcing config that impacts all dependencies / everything that goes through SystemJS. So I'm afraid that it might mess up things for more experienced SystemJs users.

I'm not against adding defaultJsExtension: true but I would prefer that we, as Angular, don't try to enforce config for an app-global / wide loader.

@IgorMinar
Copy link
Contributor Author

the thing is that the bundle should be usable even without systemjs and in that case the config doesn't do anything.

I approved the PR to remove this. If this causes more friction elsewhere we need to find a different solution.

@jpsfs
Copy link

jpsfs commented Aug 27, 2015

@pkozlowski-opensource I'm not an expert on SystemJS, please correct me if I'm wrong, but if the modules are not registered with ".js", then using defaultJsExtension: true, SystemJS will try to look for the modules with the extension and will not find them.

Actually,I think this is not happening right now because of

System.config({"paths":{"*":"*.js","angular2/*":"angular2/*"}});

When you remove it, the problem I just describe will appear.

Example:

import * from "angular2/src/di/metadata";

will trigger a search for "angular2/src/di/metadata.js" (because of the defaultJsExtension: true flag) and that will not be registered. (if you don't use the "":".js" config)

My vote would be to generate the bundle with the .js extension on the register. Something like:

System.register("angular2/src/di/metadata.js", ...);

@pkozlowski-opensource
Copy link
Member

@jpsfs I don't think it works like this. SystemJS will first consult its internal registry to see if a dependency is there. Only if a dependency is not registered SystemJS will try to fetch it and this is where defaultJsExtension comes into play.

Did you run into actual problems somewhere? If so, could you share a plunk demonstrating those?

@jpsfs
Copy link

jpsfs commented Aug 27, 2015

@pkozlowski-opensource yes, you are right. But the fact is that a dependency named "angular2/src/di/metadata.js" is not registered, only "angular2/src/di/metadata" is.

I will try to reproduce this is in a plunk asap.

@rkirov
Copy link
Contributor

rkirov commented Aug 27, 2015

@IgorMinar sounds good, lets revisit if we end up having to remind people to set defaultJsExtensions: true.

@IgorMinar
Copy link
Contributor Author

I think that using defaultJsExtensions is now discouraged. I'm looking into
this stuff this week.

On Thu, Aug 27, 2015 at 10:47 AM Rado Kirov notifications@github.com
wrote:

@IgorMinar https://github.com/IgorMinar sounds good, lets revisit if we
end up having to remind people to set defaultJsExtensions: true.


Reply to this email directly or view it on GitHub
#3826 (comment).

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants