-
Notifications
You must be signed in to change notification settings - Fork 94
feat: add devmodeModuleOverride to tsconfig bazelOptions #492
Conversation
By default devmode module format is UMD (or CommonJS if googmodule is set). This option allows that to be overridden (if googmodule is not set).
Can you motivate the change more? Why do we need more than two output formats? Can we instead replace one of the outputs with whichever one you want? |
A default of UMD makes sense for many cases in the OSS world as the devmode module format needs to be the default UMD to work with ts_devserver & karma_web_test as these both use concatjs and require a named module format. However, there are other cases that users would like to use ts_library and get a non-UMD devmode output. In the angular-cli case the devmode output .js files are put into their release packages which currently have CJS in them in their legacy build system. They can't be changed to UMD due to a typescript compilation issue with UMD. In other cases, users are not using ts_devserver or karma_web_test and would prefer a less verbose module format in their devmode .js outputs. Prodmode has to stay esm as the outputs are named .mjs. |
The typescript bug is microsoft/TypeScript#36780 and it seems to affect other usage patterns in UMD. Aside from this we want to ship I can imagine an alternative setup where instead of outputting |
We have a use case for this where we have libs that we want to consume with the angular architect bazel rule. |
Would it make sense to just obey the tsconfig's module setting then? |
That's how I originally thought it worked. I imagine there's a reason against it, but I don't know what it is. |
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.
I'd propose that it read the tsconfig and if no value is set use Howevery this is a breaking change, on the current behaviour and we couldn't release till 2.0. |
I think you could also do it with an 'obeyTsConfigModuleSetting' flag in that case. |
Ah yes, that would be a better flag, default being false to avoid break changes then consider flipping to true in 2.0 |
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.
…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.
…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.
…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.
Circled back to this. An
At that point it is not possible to know if I propose we land the |
@gregmagolan SGTM |
…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.
Since this is a build-level concern, why not specify it in the build file? |
Its possible and easier to setup for the oss ts_library since we can make that change in rules_nodejs; tho I don't have a strong argument as to why this setting is different from the other Is |
Sorry for all my questions. devmodeTargetOverride is not used in Google. (We don't let users control tsconfig at all.) I'm not sure I'm the right reviewer for this in general, I will try to find someone better. PS here is some thinking around adding options like these that I've found useful: http://neugierig.org/software/blog/2018/07/options.html |
Thanks for the link @evmar and the questions are appreciated. You brought up a good point about ts_config bazelOpts vs rule attributes. I'll give it some thought and get some feedback on that from the angular team. |
Went with bazel-contrib/rules_nodejs#1687 instead which can be handled entirely in rules_nodejs and seems like a more principled solution. |
…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.
…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.
…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.
…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.
By default devmode module format is UMD (or CommonJS if googmodule is set). This option allows that to be overridden (if googmodule is not set).
This feature is requested by @filipesilva for the angular-cli bazel migration.
PR in rules_nodejs adds test coverage for the new tsconfig setting: bazel-contrib/rules_nodejs#1687
Attention Googlers: This repo has its Source of Truth in Piper. After sending a PR, you can follow http://g3doc/third_party/bazel_rules/rules_typescript/README.google.md#merging-changes to get your change merged.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information