chore(common): resolve source instead of bundle on development #264

eunjae-lee · 2023-08-28T15:39:21Z

What?

This PR updates vite configs to resolve source files instead of bundles when NODE_ENV is not production (probably either development or test).

It means we don't have to run build command every time we update something. More details in the comments section.

For example, vite config of Container looks like this:

  resolve: {
    alias:
      process.env.NODE_ENV === 'production'
        ? []
        : [
          {
            find: /^@storyblok\/field-plugin$/,
            replacement: path.resolve(
              __dirname,
              '../field-plugin/src/index.ts',
            ),
          },
        ],
  },

It means that the source of field plugin library will be resolved instead of @storyblok/field-plugin inside the Container package. However, we cannot add such config to the templates, because we don't want this config inside users' created repositories, right? So I had to create separate vite configs for the templates. Couldn't find out an alternative to this approach.

Why?

JIRA: EXT-1751

How to test? (optional)

Screenshot.2023-08-28.at.17.03.02.mp4

vercel · 2023-08-28T15:39:25Z

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	Sep 4, 2023 8:26am

eunjae-lee · 2023-08-28T15:53:34Z

package.json

-    "dev:js": "yarn workspace field-plugin-js-template dev",
-    "dev:vue2": "yarn workspace field-plugin-vue2-template dev",
-    "dev:vue3": "yarn workspace field-plugin-vue3-template dev",
+    "dev:react": "yarn workspace field-plugin-react-template dev --config ../../dev/react-vite.config.ts",


Once this yarn workspace command is run, process.cwd() will be packages/cli/templates/react, not the root directory. So, the config path looks like that.

eunjae-lee · 2023-08-28T15:56:47Z

packages/cli/dev/js-vite.config.ts

@@ -0,0 +1,15 @@
+import { defineConfig } from 'vite'


As I said in the description of this PR, we're having this separate vite config files here, but they're actually extending the real configs, adding only this alias.

eunjae-lee · 2023-08-28T16:00:30Z

packages/cli/dev/react-vite.config.ts

@@ -0,0 +1,15 @@
+import { defineConfig } from 'vite'


I welcome any suggestion on this folder or filenames.

BibiSebi · 2023-08-29T10:12:20Z

I am wondering if the extended vite config from helper functions will actually have any effect once let's say a user creates a new template and runs yarn dev.

Quite some of the code seems to be redundant, do you think @eunjae-lee it makes sense to make the alias a reusable constant?

I am also playing with the idea if it was possible to create a more dynamic/simpler way of creating these dev configs, maybe by creating a special script. But the idea is not well-thought trough. I mean something like yarn dev-internal:react and the script would generate the correct temporary configs.

I am thinking about an easier way because every time we create a new template we need to add the new extended configuration, which might be a bit cumbersome and we will also need to add this as a new step for our contributors.

eunjae-lee · 2023-08-29T13:51:33Z

I am wondering if the extended vite config from helper functions will actually have any effect once let's say a user creates a new template and runs yarn dev.

If I get your question correctly, helper is fine. When you think about the build process, we build helpers, and copy the bundled files into packages/field-plugin/dist/... At that point, helpers' vite config doesn't matter. Let me know if it's still unclear!

I am also playing with the idea if it was possible to create a more dynamic/simpler way of creating these dev configs, maybe by creating a special script. But the idea is not well-thought trough. I mean something like yarn dev-internal:react and the script would generate the correct temporary configs.

I like the idea of a script. I didn't want to overcomplicate this pull-request from the beginning. Now that you see what's going on here, and you agree on having a better way to have this config for the sake of better maintenance, I'm happy to improve this pull-request. Ping me if you have some ideas, while I work on it :)

eunjae-lee · 2023-08-29T13:51:44Z

cc @BibiSebi
(forgot to ping you)

demetriusfeijoo · 2023-08-30T00:38:06Z

packages/field-plugin/helpers/vite/vite.config.ts

@@ -17,4 +17,15 @@ export default defineConfig({
      external: ['querystring'],
    },
  },
+  resolve: {


@eunjae-lee, I may be not seeing something here, but does it make sense to have this alias also for the vite helper?

I mean, it seems that the @storyblok/field-plugin package is not used in this helper...Am I right?

Oh, nice catch! Thanks

demetriusfeijoo · 2023-08-30T00:45:08Z

scripts/dev-template.mjs

+import fs from 'fs/promises'
+import path from 'path'
+
+const template = process.argv[3]


Suggested change

const template = process.argv[3]

const template = process.argv[3]

const templatePath = `packages/cli/templates/${template}`

const newConfigPath = `${templatePath}/node_modules/.${template}-vite.config.ts`

await $`cp ${templatePath}/vite.config.ts ${newConfigPath}`

You can select multi-lines and suggest :)

Screenshot.2023-08-30.at.10.37.24.mp4

anyway, thanks I'll apply this.

Really cool @eunjae-lee.

Thanks for sharing, it'll be helpful.

const template = process.argv[3]

Since the template parameter is mandatory for the script to work properly, I'm wondering if it would be nice to check if before and if it isn't provided, print a better error message.

What do you think @eunjae-lee ?

This script is run by bump-version, and we don't run it directly. So this script is supposed to get the argument all the time. However, I just added it for better read :)

f7726dc

demetriusfeijoo · 2023-08-30T01:12:48Z

@eunjae-lee, everything seemed nice to me.

The script file was a nice idea, I tested it locally, and worked pretty well.

I left one note for the sake of understanding better the SDK, but nothing which would break anything as well.

BibiSebi · 2023-08-30T10:54:59Z

I am wondering if the extended vite config from helper functions will actually have any effect once let's say a user creates a new template and runs yarn dev.

If I get your question correctly, helper is fine. When you think about the build process, we build helpers, and copy the bundled files into packages/field-plugin/dist/... At that point, helpers' vite config doesn't matter. Let me know if it's still unclear!

I am also playing with the idea if it was possible to create a more dynamic/simpler way of creating these dev configs, maybe by creating a special script. But the idea is not well-thought trough. I mean something like yarn dev-internal:react and the script would generate the correct temporary configs.

I like the idea of a script. I didn't want to overcomplicate this pull-request from the beginning. Now that you see what's going on here, and you agree on having a better way to have this config for the sake of better maintenance, I'm happy to improve this pull-request. Ping me if you have some ideas, while I work on it :)

What about merging this and creating another ticket to improve this? It does not have to be now, but in case we have the time and motivation we can look into it in the next sprint.
As long as it works, I think we can merge it like this, but, we need to include the information inside about the steps inside the README for contributors.
What do you think. @eunjae-lee.

BibiSebi · 2023-08-30T11:03:20Z

Never mind, I see you already started working on it :)

BibiSebi · 2023-08-30T11:09:16Z

scripts/dev-template.mjs

+
+const template = process.argv[3]
+const templatePath = `packages/cli/templates/${template}`
+const newConfigPath = `${templatePath}/node_modules/.${template}-vite.config.ts`


maybe here we also need to make a small check if node_modules is present otherwise the app might crash in the next command?

good point! but thinking of this.. if we didn't run yarn install yet, then vite is not there, and anyway nothing will work.

BibiSebi · 2023-08-30T11:12:01Z

scripts/dev-template.mjs

@@ -0,0 +1,29 @@
+#!/usr/bin/env zx
+
+import fs from 'fs/promises'


can we add a comment to explain the reasoning for this script? It might not be straightforward if a new dev joins us why we have this and inside the template, there is also a dev command so it might be confusing which one to use.

BibiSebi · 2023-08-30T11:14:05Z

packages/field-plugin/helpers/vue2/vite.config.js

@@ -22,4 +22,15 @@ export default defineConfig({
      },
    },
  },
+  resolve: {


it is totally fine for now, but the helpers will also need some kind of automated mechanism in the future, so that the developer does not need to take care of it.

demetriusfeijoo

LGTM

chore(common): resolve source instead of bundle on development

535335c

vercel bot deployed to Preview August 28, 2023 15:40 View deployment

fix lint error

674a866

vercel bot deployed to Preview August 28, 2023 15:43 View deployment

eunjae-lee commented Aug 28, 2023

View reviewed changes

eunjae-lee marked this pull request as ready for review August 28, 2023 16:01

eunjae-lee requested review from demetriusfeijoo and BibiSebi August 28, 2023 16:01

create dev vite config file on demand

72c0ea3

vercel bot deployed to Preview August 29, 2023 15:54 View deployment

demetriusfeijoo reviewed Aug 30, 2023

View reviewed changes

apply feedback

f624026

vercel bot deployed to Preview August 30, 2023 08:42 View deployment

BibiSebi reviewed Aug 30, 2023

View reviewed changes

eunjae-lee added 2 commits August 30, 2023 15:19

add comment

7632dab

Merge branch 'main' into EXT-1751-chore-field-plugin-sdk

30661d2

vercel bot deployed to Preview August 30, 2023 13:24 View deployment

eunjae-lee requested review from BibiSebi and demetriusfeijoo September 1, 2023 09:15

add error message

f7726dc

vercel bot deployed to Preview September 1, 2023 12:48 View deployment

demetriusfeijoo approved these changes Sep 1, 2023

View reviewed changes

eunjae-lee enabled auto-merge (squash) September 4, 2023 08:19

Merge branch 'main' into EXT-1751-chore-field-plugin-sdk

42bc1f3

eunjae-lee merged commit a1b4d84 into main Sep 4, 2023

eunjae-lee deleted the EXT-1751-chore-field-plugin-sdk branch September 4, 2023 08:23

vercel bot deployed to Preview September 4, 2023 08:26 View deployment

eunjae-lee mentioned this pull request Oct 27, 2023

chore(common): clean up dev vite configs #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(common): resolve source instead of bundle on development #264

chore(common): resolve source instead of bundle on development #264

eunjae-lee commented Aug 28, 2023 •

edited

Loading

vercel bot commented Aug 28, 2023 •

edited

Loading

eunjae-lee Aug 28, 2023

eunjae-lee Aug 28, 2023

eunjae-lee Aug 28, 2023

BibiSebi commented Aug 29, 2023

eunjae-lee commented Aug 29, 2023

eunjae-lee commented Aug 29, 2023

demetriusfeijoo Aug 30, 2023

eunjae-lee Aug 30, 2023

demetriusfeijoo Aug 30, 2023

eunjae-lee Aug 30, 2023

demetriusfeijoo Aug 30, 2023

demetriusfeijoo Sep 1, 2023

eunjae-lee Sep 1, 2023

demetriusfeijoo commented Aug 30, 2023

BibiSebi commented Aug 30, 2023

BibiSebi commented Aug 30, 2023

BibiSebi Aug 30, 2023

eunjae-lee Aug 30, 2023

BibiSebi Aug 30, 2023

BibiSebi Aug 30, 2023

demetriusfeijoo left a comment

		@@ -0,0 +1,29 @@
		#!/usr/bin/env zx

		import fs from 'fs/promises'

chore(common): resolve source instead of bundle on development #264

chore(common): resolve source instead of bundle on development #264

Conversation

eunjae-lee commented Aug 28, 2023 • edited Loading

What?

Why?

How to test? (optional)

vercel bot commented Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BibiSebi commented Aug 29, 2023

eunjae-lee commented Aug 29, 2023

eunjae-lee commented Aug 29, 2023

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

demetriusfeijoo commented Aug 30, 2023

BibiSebi commented Aug 30, 2023

BibiSebi commented Aug 30, 2023

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

demetriusfeijoo left a comment

Choose a reason for hiding this comment

eunjae-lee commented Aug 28, 2023 •

edited

Loading

vercel bot commented Aug 28, 2023 •

edited

Loading