Skip to content
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

Jump to bottom

perf(core): use ngDevMode to tree-shake error messages #38612

Closed
wants to merge 1 commit into from
Closed

perf(core): use ngDevMode to tree-shake error messages #38612

wants to merge 1 commit into from

Conversation

sonukapoor
Copy link
Contributor

@sonukapoor sonukapoor commented Aug 27, 2020 •
edited

This commit adds ngDevMode guard to throw some errors only in dev mode
(similar to how things work in other parts of Ivy runtime code). The
ngDevMode flag helps to tree-shake these error messages from production
builds (in dev mode everything will work as it works right now) to decrease
production bundle size.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

  • Yes
  • No

@sonukapoor sonukapoor force-pushed the feat/guard-r3-errors-with-ng-dev branch 3 times, most recently from f2e6af3 to a39e786 Compare August 28, 2020 11:07
@sonukapoor sonukapoor added the area: core Issues related to the framework runtime label Aug 28, 2020
@ngbot ngbot bot added this to the needsTriage milestone Aug 28, 2020
@sonukapoor sonukapoor force-pushed the feat/guard-r3-errors-with-ng-dev branch from a39e786 to e2247cc Compare August 28, 2020 20:42
@sonukapoor sonukapoor force-pushed the feat/guard-r3-errors-with-ng-dev branch 2 times, most recently from 566468b to a960a41 Compare August 28, 2020 22:08
@sonukapoor sonukapoor marked this pull request as ready for review August 28, 2020 22:34
@sonukapoor sonukapoor changed the title perf: use ngDevMode to tree-shake error messages perf(core): use ngDevMode to tree-shake error messages Aug 30, 2020
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Sep 1, 2020
@AndrewKushnir AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 1, 2020
AndrewKushnir
AndrewKushnir approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Thanks for making this changes @sonukapoor 👍

The changes LGTM, I just have a quick note on commit message:

  • I think we should remove a reference to the r3_injector, since shared.ts is also updated
  • Let's mention that these assertions are dev-mode-only, so adding ngDevMode will help tree-shake they away for production builds (in dev mode everything will work as it works now)

I also think it'd be great if @mhevery can have a quick look (I've added him as a reviewer) to check if it's safe to tree-shake these errors.

Thank you.

@sonukapoor
Copy link
Contributor Author

  • I think we should remove a reference to the r3_injector, since shared.ts is also updated
  • Let's mention that these assertions are dev-mode-only, so adding ngDevMode will help tree-shake they away for production builds (in dev mode everything will work as it works now)

Thanks, @AndrewKushnir - I will make the commit messages changes. In the meanwhile, can you run a presubmit?

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Sep 1, 2020
@AndrewKushnir
Copy link
Contributor

In the meanwhile, can you run a presubmit?

Sure, I've started a new presubmit and will keep you updated once I get the results. Thank you.

AndrewKushnir
AndrewKushnir approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: size-tracking

atscott
atscott approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

Reviewed-for: size-tracking

jelbourn
jelbourn approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 1, 2020
@sonukapoor sonukapoor force-pushed the feat/guard-r3-errors-with-ng-dev branch from a960a41 to aa01b6a Compare September 1, 2020 21:23
@sonukapoor
perf(core): use ngDevMode to tree-shake error messages
6caecbe 
This commit adds `ngDevMode` guard to throw some errors only in dev mode
(similar to how things work in other parts of Ivy runtime code). The
`ngDevMode` flag helps to tree-shake these error messages from production
builds (in dev mode everything will work as it works right now) to decrease
production bundle size.
@sonukapoor sonukapoor force-pushed the feat/guard-r3-errors-with-ng-dev branch from aa01b6a to 6caecbe Compare September 2, 2020 01:19
@AndrewKushnir AndrewKushnir added refactoring Issue that involves refactoring or code-cleanup action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit labels Sep 3, 2020
atscott pushed a commit that referenced this pull request Sep 8, 2020
@sonukapoor @atscott
perf(core): use ngDevMode to tree-shake error messages (#38612)
b084bff 
This commit adds `ngDevMode` guard to throw some errors only in dev mode
(similar to how things work in other parts of Ivy runtime code). The
`ngDevMode` flag helps to tree-shake these error messages from production
builds (in dev mode everything will work as it works right now) to decrease
production bundle size.

PR Close #38612
@atscott atscott closed this in 1150649 Sep 8, 2020
@sonukapoor sonukapoor deleted the feat/guard-r3-errors-with-ng-dev branch September 8, 2020 18:50
@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 Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@atscott atscott atscott approved these changes

@jelbourn jelbourn jelbourn approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sonukapoor @AndrewKushnir @mhevery @atscott @jelbourn @googlebot