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

feat: add support for TypeScript 5.0's array extends in tsconfig #1105

Merged
merged 9 commits into from
Sep 6, 2023

Conversation

smorimoto
Copy link
Contributor

@smorimoto smorimoto commented Aug 25, 2023

  • add support for TypeScript 5.0's array extends in tsconfig
  • drop support for Node.js 14
  • remove tests for Tensorflow
  • update most dependencies

Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@smorimoto smorimoto changed the title Add support for TypeScript 5.0's array extends in tsconfig feat: add support for TypeScript 5.0's array extends in tsconfig Aug 25, 2023
@smorimoto smorimoto marked this pull request as ready for review August 25, 2023 15:00
@smorimoto
Copy link
Contributor Author

smorimoto commented Aug 25, 2023

Since some dependencies had to be updated, I updated all dependencies directly linked to ncc to the latest version and then confirmed that all tests passed.

"the-answer": "^1.0.0",
"tiny-json-http": "^7.0.2",
"ts-loader": "^9.3.0",
"tsconfig-paths": "^3.7.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Can you add a test to confirm the new behavior works as expected?

…llable

Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
@smorimoto smorimoto force-pushed the typescript-5 branch 2 times, most recently from aa53d9e to 78c35df Compare August 28, 2023 18:54
@smorimoto
Copy link
Contributor Author

@styfle Done!

@smorimoto
Copy link
Contributor Author

To keep the amount of change to minimum, unrelated package updates were reverted. Let's do it with another PR as needed.

@@ -0,0 +1,8 @@
{
"extends": ["./tsconfig.a.json", "./tsconfig.b.json"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path to base configuration file to inherit from (requires TypeScript version 2.1 or later), or array of base files, with the rightmost files having the greater priority (requires TypeScript version 5.0 or later).

That is, the priorities are:

  1. tsconfig.json
  2. tsconfig.b.json
  3. tsconfig.a.json

This test verifies that the override of the allowJs property and the combination of A and B are working as expected.

@smorimoto
Copy link
Contributor Author

@styfle Could you check this?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@styfle
Copy link
Member

styfle commented Aug 30, 2023

Looks great, thanks! Running CI now :spinner2:

Signed-off-by: Sora Morimoto <sora@morimoto.io>
@smorimoto smorimoto force-pushed the typescript-5 branch 2 times, most recently from 76b7810 to a175fc1 Compare August 31, 2023 15:17
@smorimoto
Copy link
Contributor Author

@styfle The latest commits should also fix issues (not directly related to this PR!) related to native builds. I squashed the commit you added and put you in as a co-author.

@smorimoto
Copy link
Contributor Author

@styfle Additionally, branch protection appears to be enabled for the already-removed Node.js 14. Don't forget to update that as well!

@smorimoto
Copy link
Contributor Author

@styfle Could you kick the CI?

@styfle
Copy link
Member

styfle commented Sep 2, 2023

Running CI again…

@smorimoto
Copy link
Contributor Author

Oh... Windows is always a nightmare...

@smorimoto
Copy link
Contributor Author

@styfle One last poke...

@smorimoto
Copy link
Contributor Author

I think you should definitely update this configuration...

image

@smorimoto smorimoto force-pushed the typescript-5 branch 6 times, most recently from c434d1b to 3a5b299 Compare September 5, 2023 00:43
@smorimoto
Copy link
Contributor Author

@styfle I temporarily removed tfjs from the test as it was too unstable to break on other platforms when I fixed it for either platform. We can come back here again when it's correct in upstream.

smorimoto and others added 6 commits September 5, 2023 09:49
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Co-authored-by: Steven <steven@ceriously.com>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
…gyp`

Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
@smorimoto
Copy link
Contributor Author

@styfle Finally, all the tests are expected to pass, and that can be confirmed here. Now all we have to do is make sure the CI is OK here and go ahead.

- name: Use Node.js ${{ matrix.node }}
uses: actions/setup-node@v3
with:
cache: yarn
node-version: ${{ matrix.node }}
check-latest: true
- name: Install Dependencies
run: yarn install
run: yarn global add node-gyp && yarn install --frozen-lockfile
Copy link
Member

@styfle styfle Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does node-gyp need to be a global?

This might be confusing for contributors since most people do not have node-gyp installed globally.

Can we add it as a devDependency instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already listed there, but somehow it seems that Windows sometimes needs it in a global environment. I haven't investigated whether this is a problem with the package manager resolver or some kind of implementation, but generally there shouldn't be a major problem

@styfle
Copy link
Member

styfle commented Sep 5, 2023

Please update the PR description with example usage and link any existing issue, thanks!

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@styfle styfle merged commit f898f8e into vercel:main Sep 6, 2023
19 checks passed
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

🎉 This PR is included in version 0.37.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@smorimoto smorimoto deleted the typescript-5 branch September 6, 2023 18:50
@smorimoto
Copy link
Contributor Author

@styfle Whoa! Thank you for taking care of the rest of the work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants