Move reflect metadata from the polyfills to the dev environment file. #12314
Conversation
| // Used for reflect-metadata in JIT. For size and performance reasons, this should not be imported | ||
| // in AOT unless absolutely needed. All Angular metadata is removed in AOT so this is only needed, | ||
| // by default, in development. | ||
| import 'core-js/es7/reflect'; |
There was a problem hiding this comment.
I don't think the environment file is the right place to add polyfills.
Environments here are for runtime variables, and we encourage users to add API keys and such here. It's common to test your app over the production runtime environment on a development build.
We now have general purpose file replacements as well. Why not have polyfills.ts and polyfills.aot.ts and effect the replacement on AOT configurations instead? The default production configuration would replace both environment.ts and polyfills.ts.
There was a problem hiding this comment.
That's not a bad idea, but it will make the update step more complicated.
There was a problem hiding this comment.
actually since the polyfill file is an option we should just override that.
There was a problem hiding this comment.
I'm not sure we can automatically move the polyfill without breaking existing applications. Some may rely on the reflect APIs which will have mysteriously disappeared after updating.
There was a problem hiding this comment.
Yeah making a new polyfills file is much better. How about, for updating, we make a new polyfills.jit.ts, that's a copy of the existing user polyfills, but with reflect removed?
This still suffers from the problem @clydin highlighted. Maybe we should put a warning about that as well. It's probably worth some confused users to make sure most people don't ship reflect by accident.
There was a problem hiding this comment.
Update would: copy the file, remove the reflect.
New project gets a dev polyfills that imports the AOT one.
There was a problem hiding this comment.
doesn't have to be a copy. The JIT file would just contain reflect and then import the original polyfills file.
But that brings up another problem: ng build --prod --no-aot
There was a problem hiding this comment.
The ideal scenario here would be for the CLI to analyze the code and only include the polyfill if actually used. similar to babel-preset-env
hansl
added
the
needs: discussion
|
after further discussion it's likely that we'll go with #12346 instead. |
|
I'll reuse this PR for the migration |
hansl
force-pushed
the
feat-reflect-metadata
branch
from
1824394 to
ce72815
Compare
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
hansl
force-pushed
the
feat-reflect-metadata
branch
from
ce72815 to
904dc00
Compare
|
CLAs look good, thanks! |
See PR angular#12346 for more information. This PR migrates current CLI projects to the new polyfills introduced in that PR.
hansl
force-pushed
the
feat-reflect-metadata
branch
from
904dc00 to
ef32681
Compare
hansl
added
target: major
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.