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

build: add ngJitMode to ng_rollup_bundle terser config #33773

Closed

Conversation

gregmagolan
Copy link
Contributor

@gregmagolan gregmagolan commented Nov 12, 2019

This adds a tools/ng_rollup_bundle/terser_config.json file adding ngJitMode and overriding the default terser_minified config provided by the rule.

Change requested by @alxhub in bazelbuild/rules_nodejs#1338.

After this change, the layer violation in rules_nodejs can be fixed by removing "global_defs": {"ngDevMode": false, "ngI18nClosureMode": false}, from terser_config.default.json in rules_nodejs.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

I like that we are moving the config to this repo, but I have a few questions.

{
"compress": {
"global_defs": {"ngDevMode": false, "ngI18nClosureMode": false, "ngJitMode": false},
"keep_fnames": "bazel_no_debug",
Copy link
Contributor

Choose a reason for hiding this comment

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

what is "bazel_no_debug"? where does it come from? seems odd...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its part of the terser_minified rule.

If you use the magic strings "bazel_debug" or "bazel_no_debug", these will be replaced with true and false respecting the value of the debug attribute or the --define=DEBUG=1 bazel flag.

https://bazelbuild.github.io/rules_nodejs/Terser.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming is a little odd perhaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It changes the configuration based on the debug attribute here: _terser_minified(name = name + ".min_debug", src = name, debug = True, visibility = visibility)

tools/ng_rollup_bundle/ng_rollup_bundle.bzl Outdated Show resolved Hide resolved
@kara kara added the area: build & ci Related the build and CI infrastructure of the project label Nov 13, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 13, 2019
@gregmagolan gregmagolan added the area: bazel Issues related to the published `@angular/bazel` build rules label Nov 13, 2019
@gregmagolan gregmagolan force-pushed the ng_rollup_bundle_ng_jit_mode branch 2 times, most recently from 3b029d5 to 01eb4a5 Compare November 13, 2019 22:30
@gregmagolan gregmagolan force-pushed the ng_rollup_bundle_ng_jit_mode branch 2 times, most recently from bbb3a25 to f251057 Compare November 13, 2019 23:10
@gregmagolan gregmagolan added the target: patch This PR is targeted for the next patch release label Nov 14, 2019
@gregmagolan gregmagolan force-pushed the ng_rollup_bundle_ng_jit_mode branch 2 times, most recently from 78f5d7e to de8c34e Compare November 15, 2019 19:59
This adds a `tools/ng_rollup_bundle/terser_config.json` file to override the default terser_minified config provided by the rule. After this change, the layer violation in rules_nodejs can be fixed by removing `"global_defs": {"ngDevMode": false, "ngI18nClosureMode": false},` from `terser_config.default.json` in rules_nodejs.

Change requested by Alex Rickabaugh in bazelbuild/rules_nodejs#1338.
@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Nov 15, 2019
@IgorMinar IgorMinar removed the request for review from josephperrott November 15, 2019 22:06
alxhub pushed a commit that referenced this pull request Nov 15, 2019
This adds a `tools/ng_rollup_bundle/terser_config.json` file to override the default terser_minified config provided by the rule. After this change, the layer violation in rules_nodejs can be fixed by removing `"global_defs": {"ngDevMode": false, "ngI18nClosureMode": false},` from `terser_config.default.json` in rules_nodejs.

Change requested by Alex Rickabaugh in bazelbuild/rules_nodejs#1338.

PR Close #33773
@alxhub alxhub closed this in c89eed5 Nov 15, 2019
alxhub added a commit that referenced this pull request Nov 16, 2019
alxhub added a commit that referenced this pull request Nov 16, 2019
@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 Dec 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules area: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants