Skip to content

change detection behaving differently with cli build #2752

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

Closed
doubletriplezero opened this issue Oct 17, 2016 · 14 comments · Fixed by #3812
Closed

change detection behaving differently with cli build #2752

doubletriplezero opened this issue Oct 17, 2016 · 14 comments · Fixed by #3812
Labels
P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful type: bug/fix

Comments

@doubletriplezero
Copy link

OS?

Windows 7

Versions.

angular-cli: 1.0.0-beta.17
node: 6.2.1
os: win32 x64

@angular/common: 2.1.0
@angular/compiler: 2.1.0
@angular/core: 2.1.0
@angular/forms: 2.1.0
@angular/http: 2.1.0
@angular/platform-browser: 2.1.0
@angular/platform-browser-dynamic: 2.1.0
@angular/router: 3.1.0
core-js: ^2.4.1
rxjs: 5.0.0-beta.12
ts-helpers: ^1.1.1
zone.js: ^0.6.25

Repro steps.

When running the app from the angular cli build, change detection does not function as expected.

Unfortunately, our app and the situations in which this issue occurs are prohibitively complex to provide a simple way to reproduce the problem. Essentially, the change detector is not fired in all situations where we would expect it to.

Detailed explanation of the issue can be found here http://stackoverflow.com/questions/40047415/angular-2-change-detection-behaves-differently-with-cli-webpack

The log given by the failure.

No error is thrown, change detection simply fails to run when expected.

@filipesilva
Copy link
Contributor

Hm.... can you try something? In the CLI project, remove everything from src/app/polyfills.ts. Add it via CDN to index.html:

    <script src="https://unpkg.com/core-js/client/shim.min.js"></script>
    <script src="https://unpkg.com/[email protected]?main=browser"></script>
    <script src="https://unpkg.com/[email protected]"></script>

Tell me if change detection behaves the same way as the SystemJS version.

@filipesilva filipesilva added the needs: more info Reporter must clarify the issue label Oct 17, 2016
@doubletriplezero
Copy link
Author

Thank you for the reply. I'll give this a try first thing tomorrow morning when I get to the office and let you know if there's any difference.

@doubletriplezero
Copy link
Author

doubletriplezero commented Oct 18, 2016

Ok tried this and it worked as expected! And without needing to resort to timeouts or any other means of explicitly trigger change detection.

Below is what I commented out of polyfills. Same three scripts and installed versions matched.

import 'core-js/client/shim.min'; import 'zone.js/dist/zone'; import 'reflect-metadata/reflect';

So what's different here other than the "main=browser" in the querystring when pulling down zone?

I really appreciate the response and I'm glad to see this working again, but if you have any ideas as to why it wasn't working the other way I'd be interested to hear them.

Thanks again!

@doubletriplezero
Copy link
Author

doubletriplezero commented Oct 18, 2016

Just as an aside, if I import the polyfill scripts in index.html using local versions I already had in node_modules rather than pulling them down from the CDN, it still works.

I also noticed that in the latest version of the docs re: using webpack on angular.io, zone is pulled in with a require rather than an import like so:

import 'core-js/es6'; import 'core-js/es7/reflect'; require('zone.js/dist/zone');

I tried switching back to using the polyfills.ts with this code and change detection was still broken despite the require.

Also, while the angular docs show this pattern of wrapping zone in a require, the latest angular-cli still uses import on zone.js in the polyfills.ts file created by ng new.

@Puigcerber
Copy link
Contributor

Puigcerber commented Oct 19, 2016

Wow! I can confirm this issue too. Thanks @doubletriplezero for the investigation and @filipesilva for the solution cause I was resorting to ChangeDetectorRef.detectChanges() but felt hacky somehow.

@Meligy
Copy link
Contributor

Meligy commented Oct 19, 2016

Just for easy reference: If I get this correctly, the workaround can be applied to AngularCLI project by:

  1. clearing / commenting the src/polyfills.ts file
    Clearing / commenting the line that says import './polyfills.ts'; from src/main.ts (line 1)
    (Don't clear the actual src/polyfills.ts file, as it's needed for tests)

  2. Then adding the script files to the scripts array in angular-cli.json, like:

        "scripts": [
          "../node_modules/core-js/client/shim.min.js",
          "../node_modules/zone.js/dist/zone.js",
          "../node_modules/reflect-metadata/Reflect.js"
        ],
    

@doubletriplezero
Copy link
Author

I can confirm that the modification to the scripts array in angular-cli.json you suggest above does indeed work and change detection behaves normally.

The only thing I would add is that these three scripts should be placed before any others that might also be listed in the scripts array. We already had two scripts in the script array of angular-cli.json (one for our own common js functions and jQuery.js) and if they were in the array before the other three then the change detector issue still occurred.

But it seems to me that adding these to the scripts array in angular-cli.json is definitely preferred as this will do minification, bundling and tree shaking on these files, where linking the scripts inline in index.html will not.

Thanks again for your help in finding this workaround.

@doubletriplezero
Copy link
Author

I think this issue can be closed as the workaround does solve the problem, but I'm wondering if the default behavior of the cli and its use of polyfills.ts to load zone needs to be addressed.

@filipesilva
Copy link
Contributor

filipesilva commented Oct 19, 2016

Well... there was some talk of how having the zone polyfills being loaded through imports, and it's timing, might affect zones somewhat, but it was a theoretical discussion. We hadn't found a real work example where it happened. Apparently now we have.

Let me keep this open still, we'll try to find more solutions to this problem.

/cc @robwormald @Foxandxss @TheLarkInn

@filipesilva filipesilva added type: bug/fix P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed needs: more info Reporter must clarify the issue labels Oct 19, 2016
@clydin
Copy link
Member

clydin commented Oct 21, 2016

@doubletriplezero something to note is that minification and tree shaking does not currently occur for scripts

@doubletriplezero
Copy link
Author

@clydin ah ok, thanks, that's good to know.

@jaruesink
Copy link

@Meligy
Copy link
Contributor

Meligy commented Nov 4, 2016

I updated the workaround instructions above #2752 (comment) to accommodate a problem you'd face when using ng test.

In theory, you might still get weird issues in tests, because the scripts section in angular-cli.json doesn't seem to be used with the tests, so they include the polyfills using the malfunctioning way (by importing src/polyfills,ts), and do not benefit from the workaround.

filipesilva added a commit to filipesilva/angular-cli that referenced this issue Dec 31, 2016
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 1, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 2, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 2, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 3, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 3, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 20, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 20, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 20, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Jan 20, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

Fix angular#2752
Fix angular#3309
filipesilva added a commit that referenced this issue Jan 20, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

This PR loads polyfills as a separate entry point, before everything else.

Extra documentation also added to polyfills.ts.

Fix #2752
Fix #3309
Fix #4140
MRHarrison pushed a commit to MRHarrison/angular-cli that referenced this issue Feb 9, 2017
Polyfills found in polyfills.ts would not be available for scripts due to being loaded in the main bundle only.

This PR loads polyfills as a separate entry point, before everything else.

Extra documentation also added to polyfills.ts.

Fix angular#2752
Fix angular#3309
Fix angular#4140
@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants