fix: remove createRequire for package json #978

tylersayshi · 2024-10-19T14:10:34Z

vercel · 2024-10-19T14:10:37Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
waku	✅ Ready (Inspect)	Visit Preview	Oct 21, 2024 1:07am

codesandbox-ci · 2024-10-19T14:10:55Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

ojj1123

👍👍

ojj1123 · 2024-10-19T14:23:16Z

packages/create-waku/tsup.config.ts

  banner: {
    js: `import { createRequire } from 'module'; const require = createRequire(import.meta.url);`,


You removed createRequire, but Is it needed?

prompts dynamically requires readline causing this error when the banner is omitted:

Error: Dynamic require of "readline" is not supported

We could look into moving to inquirer.js from prompts:

https://github.com/SBoudrias/Inquirer.js

It seems like it is more actively maintained, so I'd imagine the esm support is more up to date and we could then remove this banner.

Yeah It feels like prompts is not almost maintained. But both of them have similar features. I wonder @dai-shi thoughts :)

Previously, I rejected the idea #339 by @himself65 because I thought it was too early.
But now that we have more active contributors, I think the time is right.
I don't think prompts is too bad, but if you guys have a better alternative, and feel like maintaining it, I'm fine of changing the dependency library, or completely re-implementing it.

I tried both enquirer and inquirer and each seems to use dynamic requires of their dependencies, so we'd need to keep this banner even if switching.

So, I don't really see a strong motivator for changing off prompts either yet.

This issue: terkelg/prompts#393 if it get's worked on should be enough to solve any of the current concerns we have now.

Yeah, That's enough since create-waku doesn't have the logic of complex CLI yet.
So I don't have enough motivation on changing other deps.

packages/create-waku/tsup.config.ts

dai-shi · 2024-10-19T22:44:39Z

packages/create-waku/src/index.ts

-  // keep original require to avoid
-  //  bundling the whole package.json by `@vercel/ncc`
-  const packageJson = createRequire(import.meta.url)('../package.json');
+  const packageJson = await import('../package.json');


So, is this syntax supported by tsup?

yep! I think esbuild is technically what supports it under the hood, but still the answer is yes, it's supported.

Actually I was wrong here ^

This errors because of the json import without the attribute, so I just changed it to use require which is defined in the banner

the error:

TypeError [ERR_IMPORT_ASSERTION_TYPE_MISSING]: Module "file:///.../waku/packages/create-waku/package.json" needs an import attribute of type "json"

himself65 · 2024-10-20T20:24:50Z

packages/create-waku/src/index.ts

-  //  bundling the whole package.json by `@vercel/ncc`
-  const packageJson = createRequire(import.meta.url)('../package.json');
+  // eslint-disable-next-line @typescript-eslint/no-require-imports
+  const packageJson = require('../package.json');


you can use JSON.parse(fs.readFileSync(..)) if wanna remove require here

Tried updating this from my phone. I think it's right but I'll need to correct the prettier issues from my laptop later haha

Thanks for the tip though!

I thought there was a motivation of preferring require to readFileSync? @himself65

Nope, I just think it’s not a familiar way to use require in ESM

#978 (comment)

packages/create-waku/src/index.ts

dai-shi

Thanks guys!!

dai-shi · 2024-10-21T03:03:05Z

Got an error:

$ npm create waku@latest            
Need to install the following packages:
  [email protected]
Ok to proceed? (y) 
✔ Project Name … waku-project
Setting up project...

Installing dependencies by running npm install...
Error: ENOENT: no such file or directory, open '../package.json'
    at Object.openSync (node:fs:603:3)
    at readFileSync (node:fs:471:35)
    at Fx (file:///Users/daishi/.npm/_npx/a0cd0342f4ca0990/node_modules/create-waku/dist/index.js:119:41) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '../package.json'
}

I think we need to use import.meta.url.

fix for #978 (comment) Also fixed an issue where the default folder name was not showing on conflict --------- Co-authored-by: Tyler <[email protected]>

fix: remove createRequire for package json

23d32e1

ojj1123 reviewed Oct 19, 2024

View reviewed changes

packages/create-waku/tsup.config.ts Show resolved Hide resolved

dai-shi reviewed Oct 19, 2024

View reviewed changes

fix: import was not working

4aa3e4b

himself65 reviewed Oct 20, 2024

View reviewed changes

Merge branch 'main' into fix/cli-require

ad722f2

vercel bot deployed to Preview October 20, 2024 22:20 View deployment

tylersayshi and others added 3 commits October 20, 2024 15:22

refactor: use json parse and readFileSync

b669d99

fix: utf8 encoding

aa282fe

fix: formatting issues

ffae929

dai-shi reviewed Oct 21, 2024

View reviewed changes

packages/create-waku/src/index.ts Outdated Show resolved Hide resolved

dai-shi added 2 commits October 21, 2024 09:53

simplify

e15eaff

Merge branch 'main' into fix/cli-require

081a26d

vercel bot deployed to Preview October 21, 2024 01:07 View deployment

dai-shi approved these changes Oct 21, 2024

View reviewed changes

dai-shi merged commit ef95f93 into dai-shi:main Oct 21, 2024
24 checks passed

tylersayshi mentioned this pull request Oct 21, 2024

fix: issues with cli reading from package.json #981

Merged

dai-shi pushed a commit that referenced this pull request Oct 21, 2024

fix: issues with cli reading from package.json (#981)

82dea92

fix for #978 (comment) Also fixed an issue where the default folder name was not showing on conflict --------- Co-authored-by: Tyler <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove createRequire for package json #978

fix: remove createRequire for package json #978

tylersayshi commented Oct 19, 2024

vercel bot commented Oct 19, 2024 •

edited

Loading

codesandbox-ci bot commented Oct 19, 2024 •

edited

Loading

ojj1123 left a comment

ojj1123 Oct 19, 2024

tylersayshi Oct 19, 2024

tylersayshi Oct 19, 2024

ojj1123 Oct 19, 2024 •

edited

Loading

dai-shi Oct 19, 2024

tylersayshi Oct 20, 2024

ojj1123 Oct 20, 2024

dai-shi Oct 19, 2024

tylersayshi Oct 20, 2024

tylersayshi Oct 20, 2024

himself65 Oct 20, 2024

tylersayshi Oct 20, 2024 •

edited

Loading

dai-shi Oct 20, 2024

himself65 Oct 21, 2024

dai-shi Oct 21, 2024

dai-shi left a comment

dai-shi commented Oct 21, 2024

		banner: {
		js: `import { createRequire } from 'module'; const require = createRequire(import.meta.url);`,

fix: remove createRequire for package json #978

fix: remove createRequire for package json #978

Conversation

tylersayshi commented Oct 19, 2024

vercel bot commented Oct 19, 2024 • edited Loading

codesandbox-ci bot commented Oct 19, 2024 • edited Loading

ojj1123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojj1123 Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylersayshi Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi left a comment

Choose a reason for hiding this comment

dai-shi commented Oct 21, 2024

vercel bot commented Oct 19, 2024 •

edited

Loading

codesandbox-ci bot commented Oct 19, 2024 •

edited

Loading

ojj1123 Oct 19, 2024 •

edited

Loading

tylersayshi Oct 20, 2024 •

edited

Loading