Skip to content

ctorParameters not stripped from BrowserPlatformLocation and TestabilityRegistry in prod builds #15194

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
IgorMinar opened this issue Jul 29, 2019 · 7 comments · Fixed by #15239
Labels
needs: more info Reporter must clarify the issue
Milestone

Comments

@IgorMinar
Copy link
Contributor

🐞 Bug report

Command (mark with an x)

- [ ] new
- [x] build
- [ ] serve
- [ ] test
- [ ] e2e
- [ ] generate
- [ ] add
- [ ] update
- [ ] lint
- [ ] xi18n
- [ ] run
- [ ] config
- [ ] help
- [ ] version
- [ ] doc

Is this a regression?

Yes, the previous version in which this bug was not present was: ....

yes

Description

A clear and concise description of the problem...

🔬 Minimal Reproduction

yarn global add @angular/cli@next
ng new test-app
cd test-app
ng build --prod

🔥 Exception or Error





If I disable terser mangling, I'll see that ctorParams survived the build optimizer scrub pass on:

            /** @nocollapse */            return BrowserPlatformLocation.ctorParameters = () => [ {
                type: void 0,
                decorators: [ {
                    type: Inject,
                    args: [ DOCUMENT ]
                } ]
            } ], BrowserPlatformLocation;

and

            TestabilityRegistry.ctorParameters = () => [], TestabilityRegistry;

This happens both with and without Ivy

🌍 Your Environment




Angular CLI: 8.2.0-rc.0
Node: 10.11.0
OS: darwin x64
Angular: 8.2.0-rc.0
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.802.0-rc.0
@angular-devkit/build-angular     0.802.0-rc.0
@angular-devkit/build-optimizer   0.802.0-rc.0
@angular-devkit/build-webpack     0.802.0-rc.0
@angular-devkit/core              8.2.0-rc.0
@angular-devkit/schematics        8.2.0-rc.0
@ngtools/webpack                  8.2.0-rc.0
@schematics/angular               8.2.0-rc.0
@schematics/update                0.802.0-rc.0
rxjs                              6.4.0
typescript                        3.5.3
webpack                           4.38.0

Anything else relevant?

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jul 30, 2019

Hi @IgorMinar, BrowserPlatformLocation and TestabilityRegistry are explicitly excluded from these optimisations.

This logic has been present since the introduction of Build Optimizer angular/devkit@3097901#diff-98ef76947c50df9f53c7f56d716cb64cR5

I don't really have enough info to know why this was the case. Maybe @filipesilva since he authored the original commit remembers the reason why that list was introduced.

@ngbot ngbot bot modified the milestone: needsTriage Jul 30, 2019
@filipesilva
Copy link
Contributor

filipesilva commented Jul 30, 2019

I don't actually remember either. The current whitelist

// Don't remove `ctorParameters` from these.
const platformWhitelist = [
'PlatformRef_',
'TestabilityRegistry',
'Console',
'BrowserPlatformLocation',
];

is retained from the precursor to Build Optimizer, which was purify+ngo .

@alxhub according to git blame you were the one that added the whitelist. Do you know if it's still needed? @kara and @IgorMinar were involved in the creation of ngo and might know as well.

Even if we don't remove it we should add context in a comment since it seems no one knows why it's important now.

@IgorMinar
Copy link
Contributor Author

unverified theory: all of these are defined as part of the platform injector which is the top most injector in an app and that injector used to have special requirements. We need to evaluate if that's still the case today.

@filipesilva
Copy link
Contributor

Removing it doesn't seem to break AIO using VE or Ivy, and it doesn't change the bundle size either.

@filipesilva
Copy link
Contributor

Asked @alxhub and he said

I believe back in the day, at runtime reflect-metadata was still load bearing for types provided by the platform injector.
I think we've done work since then to make that not the case.

So I think I'll remove the whitelist. I'll also remove the symbol based mechanism for identifying if the file is Angular Core. It only works for FESM and we've since added a flag that the caller can set that does the same. It's used on the webpack plugin, which has information about the package.

@filipesilva
Copy link
Contributor

#15217 should implement the work needed on the CLI side.

filipesilva added a commit to filipesilva/angular-cli that referenced this issue Aug 2, 2019
…gular classes

This whitelist a leftover from older Angular versions and isn't necessary anymore.

Fix angular#15194
kyliau pushed a commit that referenced this issue Aug 2, 2019
…gular classes

This whitelist a leftover from older Angular versions and isn't necessary anymore.

Fix #15194
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: more info Reporter must clarify the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants