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

feat(typescript): add devmode_target, devmode_module, prodmode_target & prodmode_module attributes #1687

Merged

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Mar 4, 2020

This allows users control over the language level & module formats produced by both the dev and prod outputs of ts_library

NB: the tsconfig bazelOpts.googmodule if set will still override the module format to CommonJS & bazelOpts.devmodeTargetOverride will trump the devmode_target attribute so this is non-breaking. In a future major release bazelOpts.devmodeTargetOverride will be removed.

Control over devmode module format is requested by @filipesilva for the angular-cli bazel migration.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content 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

WORKSPACE Outdated Show resolved Hide resolved
internal/node/_node_bin/node Outdated Show resolved Hide resolved
rules_typescript_2.patch Outdated Show resolved Hide resolved
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Mar 5, 2020
The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Mar 19, 2020
The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Mar 19, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Mar 24, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Mar 24, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
@gregmagolan gregmagolan force-pushed the module_format_override branch from 9521ee4 to 9537994 Compare March 25, 2020 02:21
@filipesilva
Copy link
Contributor

Heya, one thing I want to highlight is that in the CLI we don't want this just for devmode. We want packages to be distributed in commonjs as well.

@gregmagolan
Copy link
Collaborator Author

gregmagolan commented Mar 26, 2020

Heya, one thing I want to highlight is that in the CLI we don't want this just for devmode. We want packages to be distributed in commonjs as well.

Yup. I got that. By devmode output I'm referring the the ts_library rule's .js output which is called devmode in the rule. Prodmode outputs .mjs in oss land which is useful for rollup_bundle but less ergonomic for adding to a pkg_npm.

@gregmagolan
Copy link
Collaborator Author

Waiting on approval on bazelbuild/rules_typescript#492 before landing this so that we don't diverge from rules_typescript on this.

@filipesilva
Copy link
Contributor

@gregmagolan thanks for explaining it to me, I did not know that's what devmode meant in this context.

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Mar 26, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
@gregmagolan gregmagolan force-pushed the module_format_override branch 3 times, most recently from f7dfa41 to 5d373c0 Compare March 27, 2020 00:53
@gregmagolan gregmagolan changed the title feat(typescript): add devmodeModuleOverride to tsconfig bazelOptions feat(typescript): add devmode_target, devmode_module, prodmode_target & prodmode_module attributes Mar 27, 2020
@gregmagolan
Copy link
Collaborator Author

After discussing with Evan Martin from Google and with Alex Eagle, decided to pivot here and use oss only ts_library attributes to configure the target & module in devmode & prodmode.

The bazelOpts.devmodeTargetOverride is not used at all in google3 so it can be removed from rules_typescript in the future. The Angular repo can be updated to use the devmode_target attribute and this PR will unblock the angular-cli as they need to override the module format in devmode which they can do with devmode_module.

Matching prodmode attributes added for symmetry and consistency.

… & prodmode_module attributes

This allows users control over the language level & module formats produced by both the dev and prod outputs of ts_library
@gregmagolan gregmagolan force-pushed the module_format_override branch from 5d373c0 to 269bf37 Compare March 27, 2020 01:23
@alexeagle
Copy link
Collaborator

Still LGTM

@gregmagolan gregmagolan merged commit 1a83a7f into bazel-contrib:master Mar 27, 2020
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Mar 31, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Mar 31, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Apr 7, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
mgechev pushed a commit to angular/angular-cli that referenced this pull request Apr 7, 2020
…UMDs

The TS bug is microsoft/TypeScript#36780.

The workaround is needed because `ts_library` emits UMDs currently. This will change with bazelbuild/rules_typescript#492 and bazel-contrib/rules_nodejs#1687.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants