-
Notifications
You must be signed in to change notification settings - Fork 1
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
[WIP] Implement plop base #1249
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ef4f7e0 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 |
@@ -1,7 +1,8 @@ | |||
{ | |||
"extends": "tsconfig/library.json", | |||
"compilerOptions": { | |||
"rootDir": "." | |||
"rootDir": ".", | |||
"module": "ESNext" |
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.
To allow the (dynamic) import of node-plop
@@ -326,6 +327,16 @@ async function run() { | |||
removeEslintConfig(); | |||
} | |||
|
|||
const { default: nodePlop } = await import("node-plop"); |
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.
Normally importing nodePlop
throws an ERR_REQUIRE_ESM
error. (See #1222 (comment))
In order to successfully build with node-plop
, dynamic importing it seem to work, with the plopFile.ts
exporting using CommonJS syntax.
@jinmayamashita @seiya0914 |
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.
@ptrkdan LGTM, Thank you for your work! It's a good idea to create dynamic plops based on whether the router is selected or not.
I will continue to implement the router templates on a child branch, and whoever is available may continue to work on the package.json templates.
@ptrkdan @seiya0914
Since the packaging work is not a high priority, it doesn't need to be fully implemented here (though it might be worth raising as an issue).
This PR looks good, so it might be worth merging it first. Should we proceed with the packaging work?
@@ -7,5 +7,6 @@ export default defineConfig((options: Options) => ({ | |||
define: { | |||
"process.env.BRANCH_NAME": `"${process.env.BRANCH_NAME}"`, | |||
}, | |||
shims: 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.
https://tsup.egoist.dev/#inject-cjs-and-esm-shims
I believe it's enabled to support both ESM and CJS.
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, without it, the build produces errors since the generated build includes import.meta.url
…lerplate into feature/implement-plop-base
…lerplate into feature/implement-plop-base
…tar-lab-oss/reactjs-boilerplate into feature/implement-plop-base
…lerplate into feature/implement-plop-base
…lerplate into feature/implement-plop-base
The CLI test is still failing despite merging the latest update from It looks like it's failing on the output of |
[WIP: DO NOT MERGE YET]
Pull Request Checklist
console.log()
)npm run test:cli
)npm run test:modules
)npm run e2e
)Description
Context
Resolves #(issue)
Before
After
Notes