fix: update tsconfigs to publish types accurately

[maribethb]

The basics

• I validated my changes

The details

Resolves

Fixes #1871
Fixes problems with tsconfigs resulting in types sometimes being output to the wrong location.

I recommend reviewing by commit.

Proposed Changes

• Normalizes all tsconfigs (including those in the dev-create templates) with the following settings:
  • Sets output directory to dist instead of build, which is just straight up wrong (but was set that way in the templates).
  • Sets strict to true if it was previously true, false if it was previously not set. This makes it easier to keep track of plugins which are not currently strict. Will file follow up issues for all plugins that are not strict.
  • Sets moduleResolution to bundler.
    • This mode means we'll use the more lax resolution strategy that is used by bundlers like webpack, which we're using. We're using esm-style imports, but true esm requires you use file extensions for relative imports, e.g. import {foo} from './foo.js'. We omit the file extension and say import {foo} from './foo'. This is allowed by webpack but disallowed by strict esmodule rules. Using bundler mode makes typescript cool with us doing so.
  • Sets module to es2015. This setting affects the output of tsc. Theoretically, this shouldn't matter because we don't actually use the output of tsc directly; we're using webpack to output a UMD module currently, not an esm. I couldn't find out what the recommended value for this is if you're using ts-loader so I just went with the currently recommended best practice. n.b. I tried to set noEmit: true which would truly make this setting redundant, but ts-loader didn't like that actually.
  • Sets target to es6. This was slightly inconsistent before.
  • Does not routinely set the lib property except on the one plugin that actually needs extra libs.
  • Uses excludes rather than includes, which affects the directory structure of the final output. Also exclude the build/dist directories so TS doesn't try to compile its own output.
• Installs typescript as a devDependency in every TS plugin. It was an error that it wasn't before. The reason this is needed is because I need to make sure we're using TS 5 in the plugins, as moduleResolution: 'bundler' didn't exist before TS 5.
  • Also installed it as a devDependency in dev-scripts itself as that's the plugin running ts-loader. I found if I didn't do this, sometimes plugins ended up using the version of typescript found in the eslint-config plugin, which is just totally wrong.
• Merged in master and resolved all conflicts, so after this, the osd branch should be fully ready to be merged back into master.

Reason for Changes

• Makes tsconfigs actually correct and consistent.
• Makes type declarations output to the correct place.
• Plugins actually install their dependencies.

Test Coverage

• All builds and tests pass - except some field-date tests that already weren't passing for me on master
• Ran npm pack on 3 plugins and uploaded them to arethetypeswrong, got all green checkmarks.
• Ran publint.dev utility on 3 plugins and got green results.

Documentation

No

Additional Information

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[maribethb]

reminder to self: do not squash this PR.

cla checks are only failing on the github-actions bot that publishes (expected) and on the blocklygithubrobot account that cleans up after dependabot (unexpected?)

[BeksOmega]

Reasoning makes sense! Scanned for any glaring errors and LGTM.

I did not check for any tsconfigs or package.jsons that might have gotten missed. I'm trusting that you caught all of those :P

[alicialics]

@maribethb

n.b. I tried to set noEmit: true which would truly make this setting redundant, but ts-loader didn't like that actually.

You can use https://github.com/TypeStrong/ts-loader#compileroptions to override noEmit: true

Sets module to es2015

I think if you do this you cannot use newer es202X syntax (eg import.meta is only available in es2020). For first party typescript blockly plugins, do you want to use these at some point? If so, then the person who introduces these syntax into a ts file would need to modify the tsconfig.json in order to do so.

Does not routinely set the lib property except on the one plugin that actually needs extra libs.

A little bit surprised that DOM/DOM.Iterable is not needed

[cpcallen]

• Normalizes all tsconfigs (including those in the dev-create templates) with the following settings:

  • Sets output directory to dist instead of build, which is just straight up wrong (but was set that way in the templates).

Why is outputting to build wrong? I was under the impression that that was so that test output (*.mocha.js) would go to build/, which seems correct; equally, I understood that that ts-loader uses tsc in a way that makes this setting moot.

It seems a lot safer that manual invocation of tsc writes only to build/, so that any intermediate output files it generates do not end up in the final package.

• Uses excludes rather than includes, which affects the directory structure of the final output. Also exclude the build/dist directories so TS doesn't try to compile its own output.

It seems safer to "includes": ["src", "test"] than try to exclude everywhere else that .ts files might be found. Is your objective to change the output directory structure? If so, why?

• Installs typescript as a devDependency in every TS plugin. It was an error that it wasn't before. The reason this is needed is because I need to make sure we're using TS 5 in the plugins, as moduleResolution: 'bundler' didn't exist before TS 5.
  • Also installed it as a devDependency in dev-scripts itself as that's the plugin running ts-loader. I found if I didn't do this, sometimes plugins ended up using the version of typescript found in the eslint-config plugin, which is just totally wrong.

This seems reasonable, but did you consider the alternative of making typescripta (non-dev) dependency of dev-scripts, obviating the need to have each individual plugin depend upon it directly? This sort of makes sense because AFAICT the places tsc is actually invoked from are only in dev-scripts, and it's advantageous in terms of simplifying upgrades (because only one package needs to be upgraded), but it does mean all our packages would be obliged to use the same version of typescript.

• Merged in master and resolved all conflicts, so after this, the osd branch should be fully ready to be merged back into master.

Would it be possible to do that in a separate PR? It is difficult to properly review this PR when I don't know what's changed here vs. incidental changes in master.

[maribethb]

Sorry, I know the commits are a bit of a mess. The only non-merge commit that I made was in df4b084

I needed some changes from master before I could get the publint and arethetypeswrong tests to pass, and if those didn't pass, then it didn't make sense to do this change without being able to validate it in some way.

[maribethb]

Why is outputting to build wrong? I was under the impression that that was so that test output (*.mocha.js) would go to build/, which seems correct; equally, I understood that that ts-loader uses tsc in a way that makes this setting moot.

The .d.ts files get put into the outDir as specified in tsconfig, so it is not moot. We need the type definitions to be in dist along with everything else we package. The setting here doesn't affect the mocha build at all. That behavior is controlled by the webpack config. I confirmed this by running npm run clean (to delete build and test directories) in a plugin that has mocha tests, then npm run test and confirmed all the .mocha.js files are still in build/.

It seems a lot safer that manual invocation of tsc writes only to build/, so that any intermediate output files it generates do not end up in the final package.

We do not ever manually invoke tsc in samples afaik. Also, I believe we do clean the dist directory whenever we run webpack.

[maribethb]

It seems safer to "includes": ["src", "test"] than try to exclude everywhere else that .ts files might be found. Is your objective to change the output directory structure? If so, why?

I think I found that if I had include instead of exclude then the directory structure was mirrored in the output (i.e. all the output files were nested under dist/src/foo.js (including the d.ts files) but I want all output at the top level. However, I think that was only happening when I was manually invoking tsc to test something else, and we never manually invoke tsc, so it's fine. I will need to do some poking around, but I think you're right that just "include": ["src"] would be better.

[maribethb]

This seems reasonable, but did you consider the alternative of making typescripta (non-dev) dependency of dev-scripts, obviating the need to have each individual plugin depend upon it directly? This sort of makes sense because AFAICT the places tsc is actually invoked from are only in dev-scripts, and it's advantageous in terms of simplifying upgrades (because only one package needs to be upgraded), but it does mean all our packages would be obliged to use the same version of typescript.

I can't decide if this would be correct or not. It feels wrong. TypeScript plugins seem like they should have a dependency on TypeScript. I'll double check that if I remove it from each plugin then the correct version is still used every time from the build script, which was definitely not happening before.

[maribethb]

@alicialics

You can use https://github.com/TypeStrong/ts-loader#compileroptions to override noEmit: true

Is your suggestion to set noEmit to true in the tsconfig, then override it to false in ts-loader? If so, that is interesting, but seems a little bit convoluted. There's no actual gain to be made from setting noEmit afaik, I was just hoping to avoid having to make a decision about what the module should be.

I think if you do this you cannot use newer es202X syntax (eg import.meta is only available in es2020). For first party typescript blockly plugins, do you want to use these at some point? If so, then the person who introduces these syntax into a ts file would need to modify the tsconfig.json in order to do so.

I don't know that we're ready to start outputting es2020. I don't know that I can make a decision about supporting e.g. import.meta right now because we still have a lot of people who use blockly directly via script tags in-browser and I don't know the browser support story for new features like that. For that reason, I'm fine with making an intentional decision at the time a plugin wishes to use a es2020+ feature that can't be compiled down.

Does not routinely set the lib property except on the one plugin that actually needs extra libs.

A little bit surprised that DOM/DOM.Iterable is not needed

DOM is included by default if you don't set the lib value.

[alicialics]

Is your suggestion to set noEmit to true in the tsconfig, then override it to false in ts-loader? If so, that is interesting, but seems a little bit convoluted. There's no actual gain to be made from setting noEmit afaik, I was just hoping to avoid having to make a decision about what the module should be.

I was mainly referring to your comment that ts-loader need noEmit to work. If thats true then perhaps we should override it so that even if a 3rd party plugin author has noEmit to true for some reason their project will not fail to build. But it's a small point in any case, up to your preference.

I don't know that we're ready to start outputting es2020.

Definitely, and I am not asking for this. There are two fields, one is module and one is target. As you mentioned in the description target is set to es6 which affects output. According to https://www.typescriptlang.org/tsconfig#module the only difference between different modules for tsconfig.json are import.meta, dynamic imports and top level await. I actually misunderstood what module does so you can ignore this suggestion, as these options are fairly rare that I don't think we need to support them esp for all first party plugins. If someone needs to use it they can definitely just change tsconfig.json if needed.

I can make a decision about supporting e.g. import.meta right now because we still have a lot of people who use blockly directly via script tags in-browser

Just want to share a note on import.meta since it relates to the my other PR :) At least from my tests it would work fine in the UMD setup we have today. The dist code looks like new URL(o(183),o.b). So I am pretty sure you can just use UMD bundle as is in an old browser version. I did also test using a non-module <script> tag which definitely does not have import.meta and it falls back to document.currentScript.src assuming publicPath is set to 'auto' in webpack config. anyway completely off topic!

This seems reasonable, but did you consider the alternative of making typescript a (non-dev) dependency of dev-scripts

Might be a concern if a 3rd party plugin developer decides to install a new version of typescript and gets confused why their code won't build.

[maribethb]

I changed everything to use includes rather than excludes.

I opted not to remove TS and install it only in dev-scripts. The way dev-scripts is currently configured, it wants TypeScript as a peer dependency. I think it might be reasonable to change this, but I don't think we need to do it as part of this PR.