-
-
Notifications
You must be signed in to change notification settings - Fork 622
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
chore(build): validate if exporting is correct in package.json
and jsr.json
#3638
Conversation
It fails as follows. The test succeeds when this PR is merged. |
Fixed some missing exports |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3638 +/- ##
==========================================
+ Coverage 90.52% 91.67% +1.15%
==========================================
Files 159 159
Lines 10179 10132 -47
Branches 2827 2864 +37
==========================================
+ Hits 9215 9289 +74
+ Misses 962 841 -121
Partials 2 2 ☔ View full report in Codecov by Sentry. |
@yusukebe |
Hi @EdamAme-x ! This PR means you added a function to validate the configuration of exporting modules in |
That is what I mean. |
Thanks! Seems to be good. But adding a test for |
thanks, okay. |
@yusukebe |
@yusukebe |
package.json
and jsr.json
Hi @nakasyou Can you review this? |
It looks good to me. An idea, what do you think about generating |
|
||
const args = arg({ | ||
'--watch': Boolean, | ||
}) | ||
|
||
const isWatch = args['--watch'] || false | ||
|
||
const readJsonExports = (path: string) => JSON.parse(fs.readFileSync(path, 'utf-8')).exports | ||
|
||
const [packageJsonExports, jsrJsonExports] = ['./package.json', './jsr.json'].map(readJsonExports) |
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.
I think it also should validate typesVersions
.
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.
thanks, will work on it.
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.
@nakasyou Ah, you are right. But perhaps validating typeVersions
is another issue? We can check whether it's correct without jsr.json
.
@EdamAme-x What do you think of it?
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.
I was able to write the code, but you are right, it may be another problem.
I will consider this after this PR is merged.
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.
Okay. Let's merge this first.
Thanks.
I think there is room for consideration, let's create a new issue and discuss it there. |
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! Let's go with this. |
Related: #3636
If there is a difference between the two exports, the build will fail.