Typescript

[duaneatat]

The approach taken here is to support converting 1 file at a time.

• Vite
• Tests passing
• UMD bundling working
• Importing Plot as an ESM from another project

[duaneatat]

@mythmon I just realized that the approach I took of not including the extensions for TypeScript files will require users to use node --experimental-specifier-resolution=node as well.

So as far as extensions, the options are

1. Import without extensions. This is the recommended way of importing in TypeScript and vite is happy with it. Node, on the other hand, requires --experimental-specifier-resolution=node in order to resolve the extension-less imports. So we would have to either require users to set that flag or rewrite the imports to include the .js as part of a build step.
2. Import with the "js" extension. This works for node as well as the mocha tests, but not vite (which requires either no extension or .ts for TypeScript).
3. Import with the "ts" extension. This works for vite as well as the mocha tests, but breaks the ESM consumers since we will end up with the "ts" extensions in the dist folder (eg. import {bar} from "foo.ts";. TypeScript emits only .js files to dist, but doesn't rewrite the *.ts imports to *.js, we would have to do that ourselves as part of a build step.

I'm leaning toward 1., what do you think?

Update: I have a PR that will allow us to take Option 2 by fixing Vite so it's also happy with the .js extension for TypeScript files.

[mbostock]

Maybe I’m not understanding but it sounds like none of these options are backwards-compatible? So we’ll probably need to find another fourth option that is…

[duaneatat]

Maybe I’m not understanding but it sounds like none of these options are backwards-compatible? So we’ll probably need to find another fourth option that is…

Option 1 should be backward-compatible if we rewrote the imports to have the .js extension, right? This will avoid users having to specify --experimental-specifier-resolution=node

[Fil]

do we want to commit yarn-error.log?

[duaneatat]

do we want to commit yarn-error.log?

Nope! Will remove 🙏

[mythmon]

Maybe I’m not understanding but it sounds like none of these options are backwards-compatible? So we’ll probably need to find another fourth option that is…

Duane found the prophesied fourth option: Use .js for imports, and extend Vite to be ok with that. That should make the compiled JS files in dist/ compatible with ESM loaders, and keeps all our build tools happy. Seems like a win to me.

[Fil]

This is great! Thanks a lot @mythmon and @duaneatat

I'll need advice on how to best update the other open PRs to avoid creating too much churn.

A few other remarks/questions (of very low importance):

• I had to upgrade node (from v16.9.0 to v16.15.0), not sure why when I read package.json (which still has "engines": { "node": ">=12" }).
• The built file plot.umd.js is a bit larger (tsc outputs 4 space indent where we used to have 2, and there are a few added comments for the linters); but plot.umd.min.js is smaller (maybe because of/thanks to the change in src/curve.ts?). I guess that's expected (and OK).
• The new tests add about 10s (+50%). If there was a way to parallelize the calls to tsc eslint etc it would be nice to have. (If not, or too complicated, I realize I can use yarn test:mocha if I want to skip the linter tests.)
• There is a warning on test:mocha ((node:1632) ExperimentalWarning: --experimental-loader is an experimental feature.), which points to this warning: “Use of the feature is not recommended in production environments.”
• the tsc command (added in prepublishOnly) outputs .js files in dist/ ; this is new, and I think I understand it's there for people who might want to include individual .js files directly? Is this a pattern we support? How can we document this? Will this change from our current npm file structure?

[Fil]

It works, but the logic escapes me a little: if curve is not a curveName, the curves.get(…) must still be allowed to proceed.
For example Plot.line(data, {curve: "BASIS"}) is correct, as is Plot.line(data, {curve: {toString: () => "BASIS"}}).

[duaneatat]

👍 makes sense!

[duaneatat]

spoke to Fil about this, we can have the types be case-sensitive but still let the js be more permissive, so as it's written is good

[duaneatat]

• I had to upgrade node (from v16.9.0 to v16.15.0), not sure why when I read package.json (which still has "engines": { "node": ">=12" }).

Let me check on this

• The built file plot.umd.js is a bit larger (tsc outputs 4 space indent where we used to have 2, and there are a few added comments for the linters); but plot.umd.min.js is smaller (maybe because of/thanks to the change in src/curve.ts?). I guess that's expected (and OK).

Yeah, tsc outputs 4 spaces and there isn't a config option to change it. If this is an issue for use we can format the output code in another step.

• The new tests add about 10s (+50%). If there was a way to parallelize the calls to tsc eslint etc it would be nice to have. (If not, or too complicated, I realize I can use yarn test:mocha if I want to skip the linter tests.)

Updated so the scripts run in parallel 👍

• There is a warning on test:mocha ((node:1632) ExperimentalWarning: --experimental-loader is an experimental feature.), which points to this warning: “Use of the feature is not recommended in production environments.”

This because we use ts-node to run the mocha tests (in order to allow writing tests in typescript). Even though it warns that this is experimental, it's safe to use it for our tests.

• the tsc command (added in prepublishOnly) outputs .js files in dist/ ; this is new, and I think I understand it's there for people who might want to include individual .js files directly? Is this a pattern we support? How can we document this? Will this change from our current npm file structure?

We can no longer point package.json's "exports" to src/index.js because src is now a ts/js hybrid and not a valid es module. So we compile the ts/js into pure js that is equivalent to what used to be in src/, and save that to dist/ and update package.json to point to that location. So this will indeed change the structure: we will have more js files in dist/ and src/ is no longer pure js. Will that be an issue? If we really wanted src/ to remain unchanged, we could move all of the files in src/ to a new directory -- say ts-src/ -- and save the compiled js to src/ instead of dist/. Then we do all development in ts-src/ and src/ is made in the prepublish step (and gitignored).

[duaneatat]

I'll need advice on how to best update the other open PRs to avoid creating too much churn.

Happy to work with you on that once this branch is merged (or stabilized, and we can rebase the outstanding branches onto this one) 👍

[Fil]

Thanks for all the explanations! 4 spaces is not an issue for me.

If we now have all the compiled js files exported to dist/, I want to be clear about the implications. Does it mean that these files will become visible in npm/unpkg? Might we, then, have users who will want to rely on this (maybe because they'll want to import those files instead of the bundle)? Is this something we want to have—and want to support?

[Fil]

In d94a6ae, we started with a {} which had an undefined length. I've replaced this with undefined and the optional chaining operator (".?"). Not sure if this is correct, since the original was probably faster. I don't know how to type the original thing, though.

EDIT: solved in 30da5cb

[Fil]

In b87e8e3 I've added src/isoformat.d.ts as suggested by VSCode, but the unit tests (local and CI) fail on it.
The error in CI is Could not find a declaration file for module 'isoformat'.
(Also, the same CI detects "any" in src/isoformat.d.ts … which doesn't seem consistent with not finding that file.)

EDIT: solved in c9ed02b

[Fil]

Another issue I have with these commits is that github doesn't see that the files have been renamed, adding even more churn. Should I do an explicit git mv? (Some files seem to have moved (warnings.js => warning.ts).)

[mythmon]

Instead of using any for the rest arguments, you can use unknown. The difference is subtle, but TS will assume anything about an any type, but it won't assume anything about unknown.

[Fil]

This sounds like very arcane knowledge, black belt! I'm still a yellow belt.

[mythmon]

Here's a source that talks a bit about the difference: https://www.typescript-training.com/course/fundamentals-v3/11-top-and-bottom-types/

Some useful quotes:

We can see here that any is not always a “bug” or a “problem” — it just indicates maximal flexibility and the absence of type checking validation.

any is the way to tell Typescript that you don't know anything about the type, and that you want to back to the JS rules for types. It's useful, but anytime you do it you are weakening the type of the thing you're working with.

Values with an unknown type cannot be used without first applying a type guard

unknown is the way to tell Typescript that you don't know anything about the type, and that you would like it's help to prove that what you are doing is type safe. It's useful because you are asking Typescript to help make the types stronger.

[mythmon]

Another issue I have with these commits is that github doesn't see that the files have been renamed, adding even more churn. Should I do an explicit git mv? (Some files seem to have moved (warnings.js => warning.ts).)

Git actually doesn't have any mechanism for storing file moves (unlike some other source control systems). Every move you see in a git tool is actually a file deletion and a file addition. Tools then use various heuristics to detect moves, like looking at the proportion of lines that have changed. For many of the files we've touched so far, we are changing the majority of the lines in the file, so the heuristic flags it as not a move.

[mythmon]

We should discuss the goal for this PR: When can we merge it? I'd propose that once we have confident that the build system can produce an artifact we could publish, and that this is a route we can use to add types, we should merge this. I don't think we should wait to convert the entire repository over to TS in this PR.

We specifically designed this migration method to be parallelizable and incremental, so that we could avoid having one long lived branch that would stop all other work and/or produce difficult merge conflicts across the entire code base.

To that point, I think we may be ready or close to ready to merge this.

[Fil]

I still have this pending question: #880 (comment)

[duaneatat]

Thanks for all the explanations! 4 spaces is not an issue for me.

If we now have all the compiled js files exported to dist/, I want to be clear about the implications. Does it mean that these files will become visible in npm/unpkg? Might we, then, have users who will want to rely on this (maybe because they'll want to import those files instead of the bundle)? Is this something we want to have—and want to support?

Double-checked with Mike B about this -- src doesn't need to be a valid module, as long as it works in node when you yarn install it (which it does) then we're good to go

[mbostock]

I would like to merge and release #801 before this lands. #801 has been underway for months and I don’t want to introduce new hurdles to getting it released. Thank you for understanding.

[duaneatat]

I would like to merge and release #801 before this lands. #801 has been underway for months and I don’t want to introduce new hurdles to getting it released. Thank you for understanding.

np! We will hold off 👍

[Fil]

Now that Plot 0.5.1 is released (and no hiccups), I'd like to rebase and merge this.

[Fil]

Rebased, seems to be working.

• I have removed the parallelism in tests, which makes it harder to debug stuff. There may be a better way. I need to learn to call yarn test:mocha, yarn test:lint or yarn test:typecheck when I'm iterating
• eslint was complaining about mark = this in src/mark/delaunay.js; I told it it was OK, but maybe we could make this nicer?
• The node warning in yarn test:mocha is annoying ((node:89495) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time). Only ref I found is people fighting over it at Remove (node:86710) ExperimentalWarning: The ESM module loader is experimental. nodejs/node#30213 (comment)

@mbostock ok to merge?

[mbostock]

What does --conditions=mocha and --files do? I can’t find any documentation about these arguments.

[mbostock]

Okay, I was able to answer this myself. It’s a Node flag, not a Mocha flag, document here:

https://nodejs.org/api/packages.html#resolving-user-conditions

And it means that Node will respect the mocha export in package.json here:

plot/package.json

Line 16 in e1091a2

"mocha": "./src/index.js",

[mbostock]

And as for --files it doesn’t seem to have any effect so should probably be removed?

[mbostock]

Suggested change

	find: RegExp(`^(.*).js$`), replacement: "$1", customResolver: (importee, importer) => {
	find: RegExp(`^(.*)\.js$`), replacement: "$1", customResolver: (importee, importer) => {

[mbostock]

I pushed a few tweaks.

I’d appreciate it if someone could now take on the task of adopting Prettier so that formatting is enforced automatically.

[duaneatat]

I’d appreciate it if someone could now take on the task of adopting Prettier so that formatting is enforced automatically.

I'll enable prettier in a follow-up PR 👍 I think I'll avoid doing a bulk formatting update though, and just have it triggered per-file as changes are made to avoid too many conflicts with other branches. Sound good?