storyblok / field-plugin

Create and deploy Storyblok Field Plugin
https://www.storyblok.com/docs/plugins/field-plugins/introduction
25 stars 3 forks source link

fix(cli): replace chalk with kleur #285

Closed eunjae-lee closed 11 months ago

eunjae-lee commented 11 months ago

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[bot] commented 11 months ago

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
BibiSebi commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

ready for review @BibiSebi @demetriusfeijoo

eunjae-lee commented 11 months ago

@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!