Skip to content
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(cli): replace chalk with kleur #285

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

eunjae-lee
Copy link
Contributor

What?

npx @storyblok/field-plugin-cli@rc

After creating a monorepo, I ran yarn workspace dev and got this error message:

failed to load config from /Users/eunjae/sandbox/ext-1971/t1/packages/p1/vite.config.ts
error when starting dev server:
file:///Users/eunjae/sandbox/ext-1971/t1/node_modules/@storyblok/field-plugin/dist/vite/index.js:2
import { Chalk } from "chalk";
         ^^^^^
SyntaxError: Named export 'Chalk' not found. The requested module 'chalk' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'chalk';
const { Chalk } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)

There must be something wrong regarding ESM, but we are already successfully using another color library called kleur, so I’m going to replace chalk with kleur instead of digging into this issue.

Why?

JIRA: EXT-2038

How to test? (optional)

@vercel
Copy link

vercel bot commented Oct 16, 2023

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

Name Status Preview Comments Updated (UTC)
plugin-sandbox ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 17, 2023 8:18am

@@ -18,6 +18,9 @@ import { $ } from 'zx'
import fs from 'fs/promises'
import path from 'path'

// eslint-disable-next-line functional/immutable-data
process.env.FORCE_COLOR = '1'
Copy link
Contributor Author

@eunjae-lee eunjae-lee Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I run commands like yarn dev:vue3, the script dev-template.mjs runs the template, but it didn't display any color at all. I'm not too sure but I think there is some issue regarding color + subprocess.

https://google.github.io/zx/known-issues#colors-in-subprocess

@BibiSebi
Copy link
Contributor

BibiSebi commented Oct 16, 2023

@eunjae-lee I am having the same issue after i started testing vue 2 and ran npm run dev I got an error regarding chalk.

Screenshot 2023-10-16 at 13 07 57

@BibiSebi
Copy link
Contributor

@eunjae-lee I see that there was a change where chalk was added to the external rollup options on 22.09.2023. Could this have caused the issue? Also, I am a bit rusty on this one but could you please explain what the external means? I will not bundled in the output right?

@eunjae-lee
Copy link
Contributor Author

@eunjae-lee I see that there was a change where chalk was added to the external rollup options on 22.09.2023. Could this have caused the issue? Also, I am a bit rusty on this one but could you please explain what the external means? I will not bundled in the output right?

Yes, you're correct. external here means you're externalizing this dependency which means you don't include this library as a part of the bundle. npm install will be run, so the dependency will be anyway in the node_modules, so we don't need to include it in the bundle.

So this external seems alright to me. But... wait.. I think your error message is slightly different from mine, and I think I have a clue! (will come back)

@BibiSebi
Copy link
Contributor

@eunjae-lee I see that there was a change where chalk was added to the external rollup options on 22.09.2023. Could this have caused the issue? Also, I am a bit rusty on this one but could you please explain what the external means? I will not bundled in the output right?

Yes, you're correct. external here means you're externalizing this dependency which means you don't include this library as a part of the bundle. npm install will be run, so the dependency will be anyway in the node_modules, so we don't need to include it in the bundle.

So this external seems alright to me. But... wait.. I think your error message is slightly different from mine, and I think I have a clue! (will come back)

Strange, in my node_modules i do not see the chalk dependency.

@eunjae-lee
Copy link
Contributor Author

Strange, in my node_modules i do not see the chalk dependency.

@BibiSebi you're right!

Normally, when you run

npm install abc

Then the package abc is installed in your project. And that normally includes dist/ folder of that library. But, npm also installs dependencies from abc's package.json. So, when library authors configure their build process, they may externalize dependencies because they're installed anyway (devDependencies are not installed, btw).

That's why I thought it's okay to externalize chalk in our vite helper. However...! if we think about how we bundle our library, we build vite helper, and copy the bundled output into field-plugin/dist/vite/.... So the vite helper bundle that doesn't include chalk are inside our field plugin library bundle output. And our field-plugin's package.json doesn't include chalk as a dependency. That's why this issue is happening.

To solve this issue, we can either

  1. bundle chalk into the vite helper's bundle output (not externalize it)
    or
  2. add chalk into dependencies of the library's package.json even though it's not directly used by the library.

In my opinion, (1) is more intuitive as long as the bundle size is not an issue, and in this case, it's for @storyblok/field-plugin/vite. Not a runtime library. So I think it's okay.

So what I'd like to do is:

A) still replace chalk with kleur because, no need to use two different color libraries (we've already used kleur in the CLI)
B) include kleur into the vite helper bundle

What do you think? @BibiSebi @demetriusfeijoo

@BibiSebi
Copy link
Contributor

Thank you for the explanation, I was thinking that this might be the problem, but was unsure. It makes sense for me to go with the (1) suggestion. 👏

@eunjae-lee
Copy link
Contributor Author

ready for review @BibiSebi @demetriusfeijoo

Copy link
Contributor

@BibiSebi BibiSebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@eunjae-lee eunjae-lee merged commit 0468022 into main Oct 17, 2023
@eunjae-lee eunjae-lee deleted the EXT-2038-fix-chalk-issue-in-vite-helper branch October 17, 2023 08:49
@eunjae-lee
Copy link
Contributor Author

@demetriusfeijoo I merged this to release another version and start testing, although you haven't reviewed this yet. Let me know if you have any question or feedback!

Copy link
Contributor

@demetriusfeijoo demetriusfeijoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @eunjae-lee, thanks for letting me know, and also the detailed explanation about the issue. 🙌

I understood what happened here 😅

Thanks for fixing it 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants