feat: add devmodeModuleOverride to tsconfig bazelOptions

gregmagolan commented 4 years ago

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: https://github.com/bazelbuild/rules_nodejs/pull/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:

[x] 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
[x] 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
[x] No

Other information

evmar commented 4 years ago

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?

gregmagolan commented 4 years ago

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.

filipesilva commented 4 years ago

The typescript bug is https://github.com/microsoft/TypeScript/issues/36780 and it seems to affect other usage patterns in UMD. Aside from this we want to ship commonjs anyway because we are building node libraries exclusively.

I can imagine an alternative setup where instead of outputting commonjs from ts_library we instead output .mjs and then transpile that to commonjs. But that seems roundabout. We'd need another set of rules everywhere for it, and those tools would have to use something that can actually output commonjs, since wouldn't be available with ts_library.

Toxicable commented 4 years ago

We have a use case for this where we have libs that we want to consume with the angular architect bazel rule. When consuming UMD format via architect / build-angular it complines as an errors or wanring since UMD has dynamic require statements.

evmar commented 4 years ago

Would it make sense to just obey the tsconfig's module setting then?

filipesilva commented 4 years ago

That's how I originally thought it worked. I imagine there's a reason against it, but I don't know what it is.

Toxicable commented 4 years ago

I'd propose that it read the tsconfig and if no value is set use amd otherwise use the value set.

Howevery this is a breaking change, on the current behaviour and we couldn't release till 2.0. To amend that we could add a attribute to ts_library that mirriors the module option in the tsconfig, which will do the same as devmodeModuleOverride but mark it to be removed in 2.0 thus making the brekaing change in 2.0 but allowing this feature into 1.x

evmar commented 4 years ago

I think you could also do it with an 'obeyTsConfigModuleSetting' flag in that case.

Toxicable commented 4 years ago

Ah yes, that would be a better flag, default being false to avoid break changes then consider flipping to true in 2.0

gregmagolan commented 4 years ago

Circled back to this. An obeyTsConfigModuleSetting is non-trivial since the tsconfig used by ts_library is generated in starlark (tsconfig.bzl) and that is where the forcing of umd or esnext happens:

        # The "typescript.es5_sources" provider is expected to work
        # in both nodejs and in browsers, so we use umd in devmode.
        # NOTE: tsc-wrapped will always name the enclosed AMD modules
        # For production mode, we leave the module syntax alone and let the
        # bundler handle it (including dynamic import).
        # Note, in google3 we override this option with "commonjs" since Tsickle
        # will convert that to goog.module syntax.
        "module": "umd" if devmode_manifest or has_node_runtime else "esnext",

At that point it is not possible to know if obeyTsConfigModuleSetting is in the user's tsconfig in order to not force the module format to either umd or esnext. The same problems exists for the language level which is why we have devmodeTargetOverride instead of obeyTsConfigTargetSettings.

I propose we land the devmodeModuleOverride as it is in line with devmodeTargetOverride which already exists. Both of those are handled in tsconfig.ts at runtime where the user's tsconfig values can be parsed.

Toxicable commented 4 years ago

@gregmagolan SGTM

evmar commented 4 years ago

Since this is a build-level concern, why not specify it in the build file?

gregmagolan commented 4 years ago

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 bazelOpts that can be specified and then we'd be in the unusual spot of having a devmodeTargetOverride bazelOpt and a devmode_module_override ts_library attribute.

Is devmodeTargetOverride override used at all in google3? If not, we could remove that and add devmode_target_override to ts_library in rules_nodejs and convert angular over to the attribute fairly easily.

evmar commented 4 years ago

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

gregmagolan commented 4 years ago

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.

gregmagolan commented 4 years ago

Went with https://github.com/bazelbuild/rules_nodejs/pull/1687 instead which can be handled entirely in rules_nodejs and seems like a more principled solution.

bazelbuild / rules_typescript