Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(*): do not break when modules are used with angular-loader #14794

Closed
wants to merge 7 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Jun 16, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.

What is the current behavior? (You can also link to an open issue here)
When using angular-loader, you should be able to load the other scripts in any order. Yet, some of the external modules were assuming that window.angular would be available as soon as the script was loaded and executed. If the external module file happened to get loaded before the core angular file, things would fall apart.

What is the new behavior (if this is a feature change)?
When using angular-loader, you are be able to load the other scripts in any order.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:

gkalpak added 6 commits June 16, 2016 19:14
This could be a problem when using `angular-loader` and the `angular-sanitize` file happens to get
loaded before the core `angular` file.
This could be a problem when using `angular-loader` and the `angular-route` file happens to get
loaded before the core `angular` file.
This could be a problem when using `angular-loader` and the `angular-messages` file happens to get
loaded before the core `angular` file.
…vailable

This could be a problem when using `angular-loader` and the `angular-message-format` file happens to
get loaded before the core `angular` file.
This could be a problem when using `angular-loader` and the `angular-animate` file happens to get
loaded before the core `angular` file.
@@ -309,7 +308,6 @@ describe('HTML', function() {

describe('htmlSanitizerWriter', function() {
/* global htmlSanitizeWriter: false */
if (angular.isUndefined(window.htmlSanitizeWriter)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@Narretz
Copy link
Contributor

Narretz commented Jun 17, 2016

We should have tests to ensure this doesn't break later.

@petebacondarwin
Copy link
Contributor

@Narretz I agree. Are we now testing all the modules in a test run where the core angular source files are not loaded?

@gkalpak
Copy link
Member Author

gkalpak commented Jun 17, 2016

When we test modules, we incude the bundled angular.js file and all module source files.

What @Narretz probably means, is that we should have a test were we use angular-loader and force the module files to be loaded before the angular.js file and make sure they work.
We don't have such a test (our tests on angular-loader are pretty minimal - just testing the exposed API), that's why these issues sneaked into modules.

I'm not sure how we could force the module files to be loaded before the core angular.js file though.
(It might not be worth the trouble, since bundling all JS files seems to be a much more widespread practice than using angular-loader nowadays.)

@petebacondarwin
Copy link
Contributor

Could we set-up an e2e test that uses the loader?

@gkalpak
Copy link
Member Author

gkalpak commented Jun 17, 2016

I suppose we could (although we would probably need some extra dependencies, such as a script loader - angular-seed is using https://github.com/ded/script.js).

The thing is, if the module files aren't loaded (as in downloaded from the server) before the core angular.js file, then the tests are useless for this scenario that we want to test.

Maybe we can use the script loader to guarantee the order.

@gkalpak
Copy link
Member Author

gkalpak commented Jun 17, 2016

BTW, this would fix #9140.

@petebacondarwin
Copy link
Contributor

What if we create an e2e test that simply loads the files in the "wrong" order:

<script src="angular-loader.js"></script>
<script src="angular-resource.js"></script>
<script src="angular-animate.js"></script>
<script src="angular.js"></script>

I think that the browser ensures that these files will run in the order in which they appear in the DOM, right?

@gkalpak
Copy link
Member Author

gkalpak commented Jun 17, 2016

Obviously 😃
I'll put together an e2e test.

@gkalpak
Copy link
Member Author

gkalpak commented Jun 17, 2016

I added a commit. The test does indeed break on our current master.

@petebacondarwin
Copy link
Contributor

Great! LGTM - please merge

@gkalpak gkalpak closed this in d406a15 Jun 17, 2016
gkalpak added a commit that referenced this pull request Jun 17, 2016
…ular-loader`

Some modules used to assume that the angular helpers would always be available when their script was
executed. This could be a problem when using `angular-loader` and the module file happened to get
loaded before the core `angular.js` file.
This commit fixes the issue by delaying the access to angular helpers, until they are guaranteed to
be available.

Affected modules:
- `ngAnimate`
- `ngMessageFormat`
- `ngMessages`
- `ngRoute`
- `ngSanitize`

Fixes #9140

Closes #14794
@gkalpak gkalpak deleted the fix-loader-with-modules branch June 17, 2016 21:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants