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

fix(@schematics/angular): fix #12886, import zone related flags in other file. #13132

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@JiaLiPassion

JiaLiPassion commented Dec 4, 2018

fix #12886.

all zone related flags now in polyfills.ts, which will not work properly, because webpack will put import 'zone.js/dist/zone' before setting those flags, which will make those flags useless.

So in this PR, I created a standalone file zone-flags.ts to keep the import in correct order.

@googlebot googlebot added the cla: yes label Dec 4, 2018

@ngbot

This comment has been minimized.

ngbot bot commented Dec 4, 2018

Artifact Baseline Current Change
cli/new-production/test-project/polyfills.js 37.49KB 37.52KB +31 bytes
@alan-agius4

alan-agius4 requested changes Dec 4, 2018 edited

This LGTM.

One small nit, can you change the commit message to have the fix in the footer, so that the changelog is properly generated?

fix(@schematics/angular): import zone related flags in other file

fix #12886

Thanks.

@clydin, should be also do a migration schema to fix this?

@alan-agius4 alan-agius4 requested a review from clydin Dec 4, 2018

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:zone-poly branch from 46f4a0b to b8cb448 Dec 4, 2018

@JiaLiPassion

This comment has been minimized.

JiaLiPassion commented Dec 4, 2018

@alan-agius4, thanks for the review, commit message updated.

@angular angular deleted a comment from ngbot bot Dec 4, 2018

@clydin

Team discussion will be needed to determine whether an extra file within all new projects would be appropriate versus other potential options. (e.g., added documentation on AIO?)

@clydin

This comment has been minimized.

Member

clydin commented Dec 7, 2018

As these are not commonly set options, can you adjust the PR to add additional instructions to the existing comment regarding the need to create an additional file rather than adding an additional file to all projects?

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:zone-poly branch 2 times, most recently from de7e349 to b10b2d6 Dec 8, 2018

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:zone-poly branch from b10b2d6 to bef2fab Dec 8, 2018

@JiaLiPassion

This comment has been minimized.

JiaLiPassion commented Dec 8, 2018

@clydin, sure, thanks for the review, I updated the comment and removed the new file.

@clydin

clydin approved these changes Dec 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment