Skip to content

feat(@angular/cli): load reflect only on dev #6295

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
wants to merge 1 commit into from

Conversation

filipesilva
Copy link
Contributor

@filipesilva filipesilva commented May 12, 2017

reflect-metadata is not needed in AOT builds. It's the biggest polyfill and the cause for a significant startup time increase.

Note: this will make ng build --prod --no-aot builds for new projects not work. The workaround is to move reflect-metadata back into the common polyfills. We should discuss if this is ok.

Fix #6325

@filipesilva filipesilva force-pushed the prod-polyfills branch 2 times, most recently from 4b48ab5 to ae5eb31 Compare May 12, 2017 18:51
@filipesilva filipesilva requested a review from hansl May 13, 2017 15:00
sumitarora
sumitarora previously approved these changes May 15, 2017
@sumitarora sumitarora self-assigned this May 15, 2017
Copy link
Contributor Author

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

Blocking for now, discussion is needed regarding the --prod --no-aot break. Maybe a warning is enough.

@ishitatsuyuki
Copy link
Contributor

I think this should be done by on-the-fly replacement just like the one in bootstrapModuleFactory.

@filipesilva
Copy link
Contributor Author

@ishitatsuyuki some setups might still need it, especially if they are using decorators other than Angular.

@ishitatsuyuki
Copy link
Contributor

We can make the feature opt-in, and I don't think environment is where we should configure it. Environment is purely runtime credentials, not for build options.

@intellix
Copy link
Contributor

Does this still work? Tried this locally and with these changes my app doesn't run anymore :)
I'm building via: ng build --target=production -e ${NG_ENV:-prod} --aot
And running in OSX Chrome 59

Uncaught TypeError: jo.getOwnMetadata is not a function

@rokerkony
Copy link

rokerkony commented Jun 26, 2017

fyi:
I tried it locally and get same result as @intellix

I run it as:
npm run ng build -- -prod --aot=true --base-href=/static/frontend-som/

"@angular/*": 4.2.2
"@angular/cli": 1.1.1

`reflect-metadata` is not needed in AOT builds.
@devoto13
Copy link
Contributor

devoto13 commented Jul 5, 2017

I agree with @ishitatsuyuki that it's better be implemented as opt-in build-time replacement than putting polyfills in the environment files.

We want to get rid of this polyfill only when doing AoT, not when building for specific environment. And environment is orthogonal to AoT. In my project I have 5 environments, so it's not very convenient to paste polyfills in every environment file just to get this optimization. Also warning will be tricky to implement when running AoT build for the custom environment or without --prod meta flag.

While the obvious benefit of current implementation is simplicity I think it is wrong to abuse environment for this feature. Another point is that current implementation can be done by end user without need to eject or mess up with CLI itself.

I would rather have a flag, like --strip-reflect-polyfill, which will add noop-loader/null-loader for these two specific polyfill imports (so they won't be included in the bundle). For backwards compatibility it will be opt-in feature, but in 2.0 it will be enabled by default when AoT is enabled (breaking change) and there will be opt-out flag for people, who need this polyfills even in AoT. Like --no-strip-reflect-polyfill. Does it make sense?

@awerlang
Copy link
Contributor

awerlang commented Aug 1, 2017

Is this somehow merged already? Because on a production build reflect-metadata is not on the bundle.

"@angular/cli": "1.3.0-rc.3"
ng build --prod --aot --bo -sm --vendor-chunk=false --output-hashing=none

Notice: I removed "polyfills: "polyfills.ts"" from .angular-cli.json, in order to produce a single output file (besides inline.bundle.js).

I can't tell if this was working before, because we rely on rollup for production and thought we'd try angular-cli. Replacing reflect-metadata with core-js it "works", only the app seems to not take data from @ngrx/store, but this might be unrelated. EDIT: removing the --bo async data is working again.

@filipesilva
Copy link
Contributor Author

This isn't something we're going to do for now. It's pretty risky for projects that need Reflect. An alternative solution will be added in the future.

@filipesilva filipesilva closed this Aug 2, 2017
@filipesilva filipesilva deleted the prod-polyfills branch August 2, 2017 10:24
@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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to remove reflect-metadata polyfills from prod bundle
8 participants