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 types to config-array #3

Merged
merged 8 commits into from
May 8, 2024
Merged

feat: Add types to config-array #3

merged 8 commits into from
May 8, 2024

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Apr 24, 2024

This adds type checking to the config-array package, generating types for both ESM and CJS.

@eslint-github-bot
Copy link

Hi @nzakas!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@nzakas nzakas changed the title feat(types): Add types to config-array feat: Add types to config-array Apr 24, 2024
@@ -23,7 +23,9 @@
},
"homepage": "https://github.com/eslint/rewrite#readme",
"scripts": {
"build": "rollup -c",
"build:dedupe-types": "node ../../tools/dedupe-types.js dist/cjs/index.cjs dist/esm/index.js",
"build": "rollup -c && npm run build:dedupe-types && tsc -p tsconfig.esm.json && tsc -p tsconfig.cjs.json",
Copy link
Member

@kecrily kecrily Apr 30, 2024

Choose a reason for hiding this comment

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

We need to either delete the copied dist/{esm,cjs}/types.ts at the end of the build, or rename it to .d.ts when we copy it to avoid generating declarations of statements.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I'm not sure I follow. This outputs both types.ts and types.d.ts in the output directories, unless I'm missing something?

Copy link
Member

@kecrily kecrily May 5, 2024

Choose a reason for hiding this comment

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

Yes, types.ts is not required at publish time. And the contents of types.ts and types.d.ts are the same.

Snapshot 2024-05-05 22 14 57

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha. I think we can solve that by using files in package.json to avoid adding another step to this process.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. I think we can solve that by using files in package.json to avoid adding another step to this process.

Should we then do the same in the object-schema package?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll update all the packages in a separate PR.

@nzakas
Copy link
Member Author

nzakas commented May 3, 2024

@mdjermanovic would like your review here to get this moving.

aladdin-add
aladdin-add previously approved these changes May 6, 2024
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM. waiting @kecrily and @mdjermanovic reviewing before merging.

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@nzakas
Copy link
Member Author

nzakas commented May 7, 2024

Looks like packages aren't being properly built in CI, so I created a script to ensure packages are built in the correct order so tsc will be happy.

@kecrily
Copy link
Member

kecrily commented May 7, 2024

Removing prepare script breaks the release workflow. This is because the current workflow does not run build script before NPM publish, but instead relies on installing and executing prepare at the same time. Also JSR publish have a separate build step.

We can remove the prepare scripts for all packages and the JSR publish builds from the workflow, and then add the prepare - build script to the workspace root


But I'm sure this can be fixed in another PR

@nzakas
Copy link
Member Author

nzakas commented May 7, 2024

prepare is not going to work in this repo. I've added a build step to the release workflow to account for that.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 8b80e81 into main May 8, 2024
9 checks passed
@mdjermanovic mdjermanovic deleted the config-array-types branch May 8, 2024 09:22
@github-actions github-actions bot mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants