-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
fix: 🐛 correctly export meta
for ESM types
#374
fix: 🐛 correctly export meta
for ESM types
#374
Conversation
🦋 Changeset detectedLatest commit: a9a5bfb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this 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.
That's a very interesting bug. It's probably a bug in the bundler, but I agree that we need a workaround.
Could you please add test cases to tests/integration
where we can confirm that the correction was made?
Yeah I'll look into adding a test. I'll also look into reporting this in |
tests/integration/test.ts
Outdated
try { | ||
cp.execSync(`npx tsc`, { cwd: TEST_CWD }) | ||
|
||
assert.fail("Expect error") |
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.
Hmm. I thought the command should succeed. Was it wrong?
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.
It is passing but for some reason it's landing on the catch block. I'll take another look.
I know what's happening, i'll update.
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.
Oh, I get it! I was writing the test case incorrectly.
My previously written test case should have been:
try {
// ...
fs.writeFileSync(actualFile, `no error:${res}`)
- assert.fail("Expect error")
} catch (e: any) {
// ...
+ return
}
+ assert.fail("Expect error")
The only test case we'll probably add is:
it("should NOT fail when running tsc", () => {
cp.execSync(`npx tsc`, { cwd: TEST_CWD })
})
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.
Yeah just realize that as well, pushed a changed.
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! Thank you so much!
Not entirely sure why this is happening, but when we have:
It leads to:
Which leads to a TypeScript error:
',' expected.ts(1005)
.Changing it to:
Leads to:
Which should be the correct output.