Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bump typescript => 5.2.2; migrate to ESM #1979

Merged
merged 103 commits into from
Nov 13, 2023
Merged

Conversation

pokey
Copy link
Member

@pokey pokey commented Oct 27, 2023

Checklist

  • [-] I have added tests
  • [-] I have updated the docs and cheatsheet
  • I have not broken the cheatsheet
  • Check source maps are working for extension
  • Check source maps are working in test files
  • Don't depend on esbuild in all packages?
  • Try running vsix from CI locally
  • [-] Use two .. for update recorded tests script

@auscompgeek
Copy link
Member

Heh, I anticipated the build failing the moment I saw the PR title 😂

@auscompgeek
Copy link
Member

We need to bump typescript-eslint and prettier before typescript anyway.

@pokey
Copy link
Member Author

pokey commented Oct 27, 2023

We need to bump typescript-eslint and prettier before typescript anyway.

would you recommend doing that in a separate PR or combine with this one?

tsconfig.base.json Outdated Show resolved Hide resolved
@auscompgeek
Copy link
Member

Our typescript-eslint version disagrees with our current typescript version anyway, so we need to bump it regardless.

@pokey pokey force-pushed the pokey/bump-typescript-greater-522 branch from dd43c8d to 5b106ce Compare October 27, 2023 13:39
packages/cheatsheet-local/tsconfig.json Outdated Show resolved Hide resolved
packages/cheatsheet/jest.config.js Outdated Show resolved Hide resolved
@pokey pokey mentioned this pull request Nov 2, 2023
3 tasks
@pokey
Copy link
Member Author

pokey commented Nov 10, 2023

...and we're green!! 🙌

But do give stuff a whirl locally on windows @AndreasArvidsson to make sure it works for you 😅

(don't forget to do a pnpm install and pnpm clean)

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Nov 11, 2023

No my-ts-node still isn't working for me

> @cursorless/[email protected] preprocess-svg-hats C:\Users\andre\repositories\cursorless\packages\cursorless-vscode
> my-ts-node src/scripts/preprocessSvgHats.ts

'C:\Program' is not recognized as an internal or external command,

I'm working on a solution

@AndreasArvidsson
Copy link
Member

@pokey After I fixed the support for absolute paths in my latest commit I still can't run scripts like preprocess hats because CURSORLESS_REPO_ROOT is missing from the environmental variables

> @cursorless/[email protected] preprocess-svg-hats C:\Users\andre\repositories\cursorless\packages\cursorless-vscode
> my-ts-node src/scripts/preprocessSvgHats.ts

C:\Users\andre\repositories\cursorless\packages\common\src\testUtil\getCursorlessRepoRoot.ts:9
    throw new Error(
          ^


Error: CURSORLESS_REPO_ROOT environment variable must be set to run this script

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Nov 11, 2023

Ok that is fixed now. I must say I'm not a big fan of having to add these really complicated run scripts with env.

@pokey
Copy link
Member Author

pokey commented Nov 11, 2023

Yeah it's the whole __dirname thing. I wish esbuild supported that properly. Basically if you're using a bundler, tho, you can't really rely on it, so it's always been a bit dicey for us. We could prob look into it in a follow up. Maybe there's some esbuild plugin we could use

Maybe something like evanw/esbuild#859 (comment)

@pokey
Copy link
Member Author

pokey commented Nov 11, 2023

We could switch to using cross-spawn. That's more robust and I believe supports spaces. Or I could go back to my bash script for my-ts-node. Translating it to a node script was a Hail Mary that I thought would help windows CI and turned out to be totally irrelevant but I left it because js is a bit easier to read

@AndreasArvidsson
Copy link
Member

I definitely prefer javascript over bash

@pokey
Copy link
Member Author

pokey commented Nov 11, 2023

Yeah it's the whole __dirname thing. I wish esbuild supported that properly. Basically if you're using a bundler, tho, you can't really rely on it, so it's always been a bit dicey for us. We could prob look into it in a follow up. Maybe there's some esbuild plugin we could use

Maybe something like evanw/esbuild#859 (comment)

I guess we could also use a heuristic like looking for a .git directory and a couple child files we know should be in root

But basically the old way was pretty dicey. Eg we were relying on the fact that transpiled code was at same level of nesting, etc

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Nov 11, 2023

Yeah it's the whole __dirname thing. I wish esbuild supported that properly. Basically if you're using a bundler, tho, you can't really rely on it, so it's always been a bit dicey for us. We could prob look into it in a follow up. Maybe there's some esbuild plugin we could use
Maybe something like evanw/esbuild#859 (comment)

I guess we could also use a heuristic like looking for a .git directory and a couple child files we know should be in root

But basically the old way was pretty dicey. Eg we were relying on the fact that transpiled code was at same level of nesting, etc

meta updater just looks for pnpm-lock.yaml

@pokey
Copy link
Member Author

pokey commented Nov 11, 2023

Yeah that could work. Would make the function need to be async or I guess just read synchronously

Or we could bake computing the env var into the my-ts-node script now that we own it

@pokey
Copy link
Member Author

pokey commented Nov 11, 2023

ok @AndreasArvidsson have a look at my latest changes and lmk if things still work locally for you; I

  • switched from vanilla node spawn to cross-spawn in an effort to make spawning more robust,
  • set CURSORLESS_REPO_ROOT from my-ts-node
  • added lint rule to ensure we keep up with all the esm stuff

@AndreasArvidsson
Copy link
Member

Works fine here

@pokey
Copy link
Member Author

pokey commented Nov 11, 2023

great! have a look at the code when you get a minute; it's not nearly as long as the diff implies. the vast majority of the lines of code are in the pnpm lockfile. If you're happy, let's give @auscompgeek a chance to have a look and then let's ship it!

@pokey
Copy link
Member Author

pokey commented Nov 13, 2023

ok @auscompgeek I'm going to go ahead and merge this one in, but feel free to leave comments when you get a minute and I can fix in follow-ups. Biggest remaining annoyance from my perspective is that every test file (eg foo.test.ts) gets bundled into its own output file including all of its dependencies, so locally you end up with a dir of generated files that's about 140MB

@pokey pokey added this pull request to the merge queue Nov 13, 2023
Merged via the queue into main with commit 7efcbfd Nov 13, 2023
14 checks passed
@pokey pokey deleted the pokey/bump-typescript-greater-522 branch November 13, 2023 13:51
@pokey pokey changed the title bump typescript => 5.2.2 bump typescript => 5.2.2; migrate to ESM Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants