-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
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.
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 |
@@ -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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
inpackage.json
to avoid adding another step to this process.
Should we then do the same in the object-schema
package?
There was a problem hiding this comment.
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.
@mdjermanovic would like your review here to get this moving. |
There was a problem hiding this 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>
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. |
Removing We can remove the But I'm sure this can be fixed in another PR |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This adds type checking to the
config-array
package, generating types for both ESM and CJS.