-
Notifications
You must be signed in to change notification settings - Fork 18
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
Generator emits cjs #1138
Generator emits cjs #1138
Conversation
3dcf0ea
to
cd1f78b
Compare
a3ebb46
to
25d4163
Compare
target: ScriptTarget.ES2020, | ||
moduleResolution: ModuleResolutionKind.NodeNext, | ||
resolvePackageJsonExports: true, |
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.
So you have to explicitly set this now because you're not specifying moduleResolution anymore?
More concretely, if module is CommonJS, then moduleResolution becomes node10, in which case, it wont look at exports by default. But if its ES2022, then technically you don't need to explicitly set that line right cuz it'll default on?
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.
Correct. So I want to be sure we are generating in a way that will work with Node16 so I turn this on.
@@ -521,567 +521,567 @@ describe("generator", () => { | |||
expect( | |||
tweakedFilesForSnapshotConsistency(helper.getFiles()), | |||
).toMatchInlineSnapshot(` | |||
{ |
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.
is the main change here that now all the imports and exports have .js
?
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, it just simplified things and I wrote a test project to be sure it didn't matter
@@ -42,21 +42,29 @@ export async function generatePackageJson(options: { | |||
const packageJson = { | |||
name: options.packageName, | |||
version: options.packageVersion, | |||
main: "./index.js", | |||
types: "./index.d.ts", | |||
main: "./cjs/index.js", |
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.
So are these two fields cjs now because node10 default looks at the main field?
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
@@ -68,7 +68,7 @@ jobs: | |||
|
|||
build: | |||
name: Build and Test | |||
timeout-minutes: 15 | |||
timeout-minutes: 20 |
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.
Dang our tests taking long now
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.
Left some questions above, but approving to unblock
PR is broken into 3 commits and I recommend you review them by commit.
.js
extensions everywhere