mosaic-sql: typescript implementation #274

calvinfo · 2024-01-24T00:32:51Z

I took a stab at updating the mosaic-sql package to use typescript. Here's the implementation.

My thinking for why to start with just mosaic-sql.

as the 'base' for more of the stuff in core, this one seemed like a good place to start
it seemed like the most well-tested of the packages

I tried to keep all the tests the same, though there are a few key points to bring up...

tests/cast-test.ts: the labels are now avg("bar") rather than avg(bar). I think this is the correct behavior, but let me know if it's not? I wasn't exactly sure what these labels were used for.
the package.json for mosaic-sql exports dist/index.js as the main field. this allows core to import regular js files in testing (though better would be to use the index.ts down the road
esbuild.js is modified so that it now takes an argument. this way the typescript generation can be configured on a package-by-package basis

What do you guys think of it? I'm not sure I have enough time to focus on some of the other packages quite yet, but wanted to get this out there as a starting point. Some of the types are quite 'loose', and I think a second pass to both simplify and unify them would be really beneficial.

I am happy to keep this in PR form for others to pick up, but wanted to get it out there as a starting point.

-Calvin

This commit updates mosaic-sql to use typescript. The functionality should be the same

jheer · 2024-01-24T00:40:59Z

Thanks @calvinfo! I'll take a closer look when I have some time later on. For reference, the label is a text string that can be used by visualization code to generate human-friendly labels (e.g., as part of axis titles). FWIW the lack of quotation marks was intentional.

calvinfo · 2024-01-24T00:42:18Z

Oooh got it. Okay, I can fix that. Will also fix the broken linting right now.

domoritz

Very nice. Looking forward to getting this in.

I like that we can do this in steps rather than everything at once.

domoritz · 2024-01-24T18:33:22Z

packages/sql/tsconfig.json

+ "forceConsistentCasingInFileNames": true,
+ "noEmit": true,
+ "esModuleInterop": true,
+ "module": "CommonJS",


Why commonjs? Should we use esm?

Yeah, good call! Can make changes there

domoritz · 2024-01-24T18:33:42Z

packages/sql/tsconfig.json

@@ -0,0 +1,29 @@
+{
+ "compilerOptions": {
+ "target": "es6",


I think we can target esnext with our bundler, no?

domoritz · 2024-01-24T18:33:49Z

packages/sql/tsconfig.json

+ "allowJs": true,
+ "skipLibCheck": true,
+ "strict": true,
+ "forceConsistentCasingInFileNames": true,


Can remove this

domoritz · 2024-01-24T18:34:03Z

packages/sql/tsconfig.json

+ "moduleResolution": "node",
+ "resolveJsonModule": true,
+ "isolatedModules": true,
+ "jsx": "preserve",


We don't use jsx.

domoritz · 2024-01-24T18:34:14Z

packages/sql/tsconfig.json

+ "resolveJsonModule": true,
+ "isolatedModules": true,
+ "jsx": "preserve",
+ "incremental": true,


I think we can use the default.

domoritz · 2024-01-24T18:35:40Z

packages/sql/tsconfig.json

+ "esnext"
+ ],
+ "allowJs": true,
+ "skipLibCheck": true,


Is there an error in a lib?

domoritz · 2024-01-24T18:35:48Z

packages/sql/tsconfig.json

+ "dom.iterable",
+ "esnext"
+ ],
+ "allowJs": true,


Do we have js?

not now, can remove :)

domoritz · 2024-01-24T18:36:50Z

esbuild.js

- entryPoints: ['src/index.js'],
- target: ['esnext'],
- format: 'esm',
+ entryPoints: [entrypoint],


Suggested change

entryPoints: [entrypoint],

entryPoints: [entrypoint ?? "src/index.js"],

then we don't need to pass it in the build calls below

ah great idea

domoritz · 2024-01-24T18:37:37Z

package.json

@@ -19,16 +19,19 @@
 "test": "lerna run test",
 "server": "nodemon packages/duckdb/bin/run-server.js",
 "dev": "vite",
- "release": "npm run test && npm run lint && lerna publish"
+ "release": "npm run build && npm run test && npm run lint && lerna publish"


Could we run tests with ts-node so we don't need to build?

Looks like we use ts-mocha so this should not be needed, right?

@domoritz the reason I'd added was because the mosaic-core package relies on this library for testing. my thinking was that running the build first would ensure that mosaic-core is referencing the dist file, but maybe that's not right?

Oh I see. Maybe we should run the tests everywhere with ts-mocha so that we can use ts or js. I am worried that we compiled and raw code gets out of sync and devs get confused.

yeah, that's a solid idea. I think it'd be easy enough to add too

domoritz · 2024-01-24T18:38:14Z

packages/sql/package.json

- "build": "node ../../esbuild.js mosaic-sql",
- "lint": "eslint src test --ext .js",
- "test": "mocha 'test/**/*-test.js'",
+ "prebuild": "rimraf dist && mkdir dist && tsc",


Why run tsc here? Doesn't esbuild work with ts?

good call, this is from an earlier version, will remove!

calvinfo · 2024-01-24T19:02:09Z

One other question for both of you... in terms of linting, I think we could either do...

add @typescript-eslint to the .eslint config for this repo. using the recommend plugins means getting rid of all any types (or ignoring them manually). I've considered adding an Expression type which would be type Expression = SQLExpression | Ref | Param | string; which I think would take care of the majority of cases. I want to ensure I'm understanding the typing properly... does that seem right?
remove linting for this package for the moment, since the typescript compilation will catch any obvious mistakes.

calvinfo · 2024-01-24T21:45:35Z

Also just followed up with a cleaned tsconfig. Let me know what you think.

domoritz · 2024-01-24T21:48:48Z

eslint does more than tsc so I think it's worth having both.

calvinfo · 2024-01-26T00:48:11Z

Okay, label now works as desired (regarding the above comment) and npm run lint is fixed too. For the moment it warns on any types, but I think we can narrow those down over time. I could definitely benefit from both your thoughts on the typing side when it comes to making them much tighter and more intuitive.

domoritz · 2024-01-26T12:30:20Z

packages/sql/package.json

- "module": "src/index.js",
+ "types": "dist/mosaic-sql.d.ts",
+ "main": "dist/mosaic-sql.js",
+ "module": "src/index.ts",


Is this ok? I thought we would need to always point to js files here.

You're right, will address.

domoritz · 2024-01-26T12:31:29Z

packages/sql/src/aggregates.ts

@@ -0,0 +1,161 @@
+import { SQLExpression, parseSQL, sql } from "./expression";


Is that any way you can mark the files as moved? Maybe rename first and commit and then change the content. It would make it easier to see the differences.

I agree, I'm a little time-strapped for the next few days but will see if I can do some sort of rebase + fix commit log to make it easier to compare the diff.

Maybe the easiest is to make a new branch, do the rename, commit, paste in the new files, commit again. Replace this branch.

However, now that I think about it, I wonder whether renames will be tracked correctly when we squash.

I'm kind of surprised that GitHub doesn't show the files as renamed. Are there so many changes?

@domoritz I have a cleaned up version here: https://github.com/uwdata/mosaic/compare/main...calvinfo:calvinfo/sql/ts-2?expand=1. Lmk what you think. I'm happy to make a PR to that if you think it looks good.

Thanks. It still looks like some of the files are not tracked as renamed like packages/sql/src/datetime.js.

Yeah, not sure what to tell you there. I separated it out this time into two explicit rename commits (no functionality change)

37a2b92
bc4d19c

In the case of datetime.js, I think a high enough percentage of the lines changed to trigger a "re-write" vs a "rename" in the github UI.

Thanks for separating the changes. Once this works, let's merge two commits. One with all the renames, and one with the code changes. That should be good!

calvinfo · 2024-02-01T19:01:18Z

Tried using this in my own packages, and I think there are definitely some quirks when packaging for isomorphic use (and I think I got some of the typing wrong even though the tests pass). Definitely don't merge yet unless you want to take a better stab at updating the package.json

domoritz · 2024-02-08T18:24:57Z

@calvinfo I'd love to merge this to avoid future conflicts. I think it's okay if the types are not fully correct yet but we need clean support for having js and ts packages and an easy way to have changes to the ts sources propagate. Could I help you push this over the finish line?

calvinfo · 2024-02-09T00:13:31Z

@domoritz please! Any help is super appreciated, feel free to tweak this and run with it. I can put together a list of issues I've noticed trying to pull in this package elsewhere. I'm a little swamped over the next week to make changes and don't want to block the new refactors y'all are trying to push through.

eknowles · 2024-04-24T07:33:39Z

Is it intentional to delete/create new files rather than rename with git? I feel there's a bit of git history being lost there.

domoritz · 2024-04-24T14:56:38Z

Is it intentional to delete/create new files rather than rename with git? I feel there's a bit of git history being lost there.

#274 (comment)

domoritz · 2024-04-24T14:57:09Z

Closing for now since we have #355 with much better ts support.

calvinfo · 2024-04-24T15:04:05Z

Thanks @domoritz! Excited to give it a shot when I pick up the project again.

willium · 2024-04-28T19:15:20Z

I put these partial declarations together for my own needs while waiting for #355 to land

calvinfo added 3 commits January 23, 2024 15:46

mosaic-sql: converting to typescript

59d561a

This commit updates mosaic-sql to use typescript. The functionality should be the same

removing bad config

17f37c6

fixing column order

272e601

domoritz requested changes Jan 24, 2024

View reviewed changes

addressing tsconfig comments

2817453

calvinfo added 3 commits January 25, 2024 16:35

aggregate: fixing label, fixing tests

4445de3

eslint: adding typescript-specific .eslintrc

504416f

package.json: adding @typescript-eslint packages

9259b0e

domoritz reviewed Jan 26, 2024

View reviewed changes

domoritz mentioned this pull request Mar 4, 2024

Add TypeScript Support #312

Open

domoritz closed this Apr 24, 2024

	entryPoints: [entrypoint],
	entryPoints: [entrypoint ?? "src/index.js"],

		@@ -0,0 +1,161 @@
		import { SQLExpression, parseSQL, sql } from "./expression";

mosaic-sql: typescript implementation #274

mosaic-sql: typescript implementation #274

Conversation

calvinfo commented Jan 24, 2024

jheer commented Jan 24, 2024

calvinfo commented Jan 24, 2024

domoritz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoritz Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinfo commented Jan 24, 2024

calvinfo commented Jan 24, 2024

domoritz commented Jan 24, 2024

calvinfo commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinfo commented Feb 1, 2024

domoritz commented Feb 8, 2024

calvinfo commented Feb 9, 2024

eknowles commented Apr 24, 2024

domoritz commented Apr 24, 2024

domoritz commented Apr 24, 2024

calvinfo commented Apr 24, 2024

willium commented Apr 28, 2024

domoritz left a comment •

edited

Loading

domoritz Jan 24, 2024 •

edited

Loading