feat(cli): add opt-in experimental support for Vite bundler #2568

emmenko · 2022-04-25T10:25:02Z

(tech time work)

Background

The mc-scripts CLI uses Webpack as the bundler for development and for building the production assets.

Webpack traditionally requires a lot of configuration and it's also not one of the fastest and more performant tools, especially in big projects. This usually results in a frustrating developer experience.

Proposal

Vite takes a different and more modern approach in bundling your application, primarily around new features like native ES modules.

Why Vite? Read here: https://vitejs.dev/guide/why.html

This PR adds opt-in support (by setting the environment variable ENABLE_EXPERIMENTAL_VITE_BUNDLER="true") to switch the bundler used for developing and building production bundles.

All the existing CLI commands fully work the same as before, so there is no migration step required from users to use the new bundle.

If you want to try it out, simply turn on the flag and enjoy 🚀

For comparison: build time seems to be cut in half on average (left with Webpack, right with Vite). 😎

changeset-bot · 2022-04-25T10:25:05Z

🦋 Changeset detected

Latest commit: e2396e1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@commercetools-frontend/mc-dev-authentication	Minor
@commercetools-frontend/mc-html-template	Minor
@commercetools-frontend/mc-scripts	Minor
merchant-center-application-template-starter	Patch
playground	Patch

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

vercel · 2022-04-25T10:25:05Z

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

Name	Status	Preview	Updated
merchant-center-application-kit	✅ Ready (Inspect)	Visit Preview	Apr 27, 2022 at 2:45PM (UTC)

tdeekens

Really promising. Thanks for spending time on this!

Just as a general question: is your vision for the mc-scripts and bundling in general that app-kit always supports the (or one) most commonly used bundler while from time to time we transition to it or would you also see supporting different bundlers at the same time on the long-term?

tdeekens · 2022-04-25T11:36:24Z

.changeset/fair-poets-watch.md

+'@commercetools-frontend/mc-scripts': minor
+---
+
+Enable opt-in support for experimental bundler (using [Vite.js](https://vitejs.dev/)). To enable it, set the environment variable `ENABLE_EXPERIMENTAL_BUNDLER="true"` in your dotenv file.


Suggested change

Enable opt-in support for experimental bundler (using [Vite.js](https://vitejs.dev/)). To enable it, set the environment variable `ENABLE_EXPERIMENTAL_BUNDLER="true"` in your dotenv file.

Enable opt-in support for experimental bundler (using [Vite.js](https://vitejs.dev/)). To enable it, set the environment variable `ENABLE_EXPERIMENTAL_VITE_BUNDLING=true"` in your dotenv file.

Just as we might have the flag for a bit to make it really clear what it turns on.

emmenko · 2022-04-25T21:20:45Z

is your vision for the mc-scripts and bundling in general that app-kit always supports the (or one) most commonly used bundler while from time to time we transition to it or would you also see supporting different bundlers at the same time on the long-term?

Good question. I probably wouldn't support multiple different bundles. At least I don't see a need for that.
As long as we can abstract away (most of) the implementation details (the mc-scripts CLI usage remains the same) it does not matter much what bundler we use internally.

So in this case, if we are happy with Vite we will then make it the default and remove the Webpack stuff.

tdeekens · 2022-04-26T05:52:54Z

is your vision for the mc-scripts and bundling in general that app-kit always supports the (or one) most commonly used bundler while from time to time we transition to it or would you also see supporting different bundlers at the same time on the long-term?

Good question. I probably wouldn't support multiple different bundles. At least I don't see a need for that.

As long as we can abstract away (most of) the implementation details (the mc-scripts CLI usage remains the same) it does not matter much what bundler we use internally.

So in this case, if we are happy with Vite we will then make it the default and remove the Webpack stuff.

I frankly also do not see the need. I just remembered us having a discussion around it being an option a while ago. Supporting multiple in the end costs maintenance and it would be hard to weigh the trade offs and allow users make informed decisions. So I think recommending one is probably easiest for both sides.

emmenko · 2022-04-26T15:14:46Z

packages/mc-dev-authentication/middlewares/create-mc-dev-authentication-middleware.js

+  path.join(__dirname, '../views/logout.pug')
+);
+
+function createMcDevAuthenticationMiddleware(applicationConfig) {


This is an "aggregated" middleware for handling login/logout requests from developing against the local MC API.

This is only useful for internal teams.

emmenko · 2022-04-26T15:15:53Z

packages/mc-dev-authentication/middlewares/create-mc-dev-authentication-middleware.js

+  const htmlLogin = compileLoginView({ env: applicationConfig.env });
+  const htmlLogout = compileLogoutView({ env: applicationConfig.env });


Instead of relying of writing/reading these pages from disk, we can simply handle it in memory.

Here we compile the pages and serve them when needed.

emmenko · 2022-04-26T15:16:20Z

packages/mc-dev-authentication/middlewares/index.js

 const createLoginMiddleware = require('./create-login-middleware');
 const createLogoutMiddleware = require('./create-logout-middleware');


These two are kept for backwards compatibility

emmenko · 2022-04-26T15:16:49Z

packages/mc-dev-authentication/transformer-local.js

-  fs.copyFileSync(
-    path.join(__dirname, 'views', 'login.css'),
-    path.join(paths.appBuild, 'login.css')
-  );
-  fs.copyFileSync(
-    path.join(__dirname, 'views', 'login.js'),
-    path.join(paths.appBuild, 'login.js')
-  );
-  fs.copyFileSync(
-    path.join(__dirname, 'views', 'logout.js'),
-    path.join(paths.appBuild, 'logout.js')
-  );


This was never really necessary, as the CSS and JS are included in the HTML page.

Odd. Maybe a left over from a refactoring. Thanks for removing.

emmenko · 2022-04-26T15:17:30Z

packages/mc-html-template/html-docs/application.html

@@ -0,0 +1,72 @@
+<!DOCTYPE html>


Moving this in to a proper HTML file makes it a bit "easier" to maintain and read (syntax highlighting) instead of being a huge block of string.

emmenko · 2022-04-26T15:18:06Z

packages/mc-html-template/html-scripts/public-path.js

+window.__dynamicImportHandler__ = function (importer) {
+  return `${window.app.cdnUrl.replace(/\/$/, '')}/${importer.replace(
+    /^(\.\/)?/,
+    ''
+  )}`;
+};
+window.__dynamicImportPreload__ = function (preloads) {
+  return preloads.map(
+    (preload) => `${window.app.cdnUrl.replace(/\/$/, '')}/${preload}`
+  );
+};


This is the equivalent of Webpack's __webpack_public_path__.

emmenko · 2022-04-26T15:18:28Z

packages/mc-html-template/src/generate-template.js

-  );
-
-  return `
-<!DOCTYPE html>


Move to a separate HTML file.

emmenko · 2022-04-26T15:20:55Z

packages/mc-html-template/src/load-html-docs.js

+// @preval
+/**
+ * https://github.com/kentcdodds/babel-plugin-preval#preval-file-comment--preval
+ * NOTE: This file is pre-evaluated during build time, using `babel-plugin-preval`.
+ * This is ok as the loaded files are static anyway and it prevents possible
+ * loading issues when files are required through Webpack own context.
+ */


This is important and it's something we could have done some time ago.

Essentially instead of shipping the actual HTML, CSS, JS snippets, we bundle them inline on compile time.

This is how this file looks:

Trying to remember why we didn't do this but can't really. Maybe just cause preval wasn't really a thing back then.

emmenko · 2022-04-26T15:22:21Z

packages/mc-scripts/src/commands/build-vite.js

+    transformIndexHtml(rawHtml, _ctx) {
+      // Ensure to use the `cdnUrl` value when loading the entry point.
+      // NOTE: with Webpack this is done by setting the `__webpack_public_path__`.
+      const html = rawHtml.replace(
+        new RegExp(`<script type="module"(.*) src="(.*)">`, 'g'),
+        `<script type="module"$1 src="__CDN_URL__$2">`
+      );
+
+      return {
+        html,
+        tags: [
+          {
+            tag: 'script',
+            // Inject the functions to dynamically change the public path.
+            // This is also used for the `cdnUrl` and is the equivalent
+            // of Webpack's `__webpack_public_path__`.
+            children: htmlScripts.publicPath,
+            injectTo: 'body',
+          },
+        ],
+      };


This transformer is important and it allows to prepend the asset URLs with the cdnUrl value.

emmenko · 2022-04-26T15:22:43Z

packages/mc-scripts/src/commands/build-vite.js

+  // TODO: allow to pass additional config options.
+  // * `define`
+  // * `plugins`


This can be done as a follow up.

emmenko · 2022-04-26T15:23:32Z

packages/mc-scripts/src/commands/start.js

-      allowedHost: urls.localUrlForBrowser,
-      contentBase: paths.appBuild,


Those weren't really used.

tdeekens

Super nice. Did a first review. Not much to add. Will however need another round with a fresh mind tomorrow.

tdeekens · 2022-04-26T17:45:46Z

packages/mc-dev-authentication/transformer-local.js

-  fs.copyFileSync(
-    path.join(__dirname, 'views', 'login.css'),
-    path.join(paths.appBuild, 'login.css')
-  );
-  fs.copyFileSync(
-    path.join(__dirname, 'views', 'login.js'),
-    path.join(paths.appBuild, 'login.js')
-  );
-  fs.copyFileSync(
-    path.join(__dirname, 'views', 'logout.js'),
-    path.join(paths.appBuild, 'logout.js')
-  );


Odd. Maybe a left over from a refactoring. Thanks for removing.

tdeekens · 2022-04-26T17:46:10Z

packages/mc-html-template/html-docs/application.html

@@ -0,0 +1,72 @@
+<!DOCTYPE html>


tdeekens · 2022-04-26T17:50:39Z

packages/mc-html-template/html-scripts/public-path.js

@@ -0,0 +1,11 @@
+window.__dynamicImportHandler__ = function (importer) {


Maybe we can add a comment to the file what this does and why its needed for our future selfs? I can follow along but wonder if we won't forget in a year or two.

Update: after reading the whole PR the comments are sufficient. It's just that one when reviewing ends up here first :)

tdeekens · 2022-04-26T17:53:58Z

packages/mc-html-template/src/load-html-docs.js

+// @preval
+/**
+ * https://github.com/kentcdodds/babel-plugin-preval#preval-file-comment--preval
+ * NOTE: This file is pre-evaluated during build time, using `babel-plugin-preval`.
+ * This is ok as the loaded files are static anyway and it prevents possible
+ * loading issues when files are required through Webpack own context.
+ */


Trying to remember why we didn't do this but can't really. Maybe just cause preval wasn't really a thing back then.

emmenko · 2022-04-27T10:29:50Z

TODOs:

Rebase PR and resolve conflicts.
Some assets are not correctly loaded in production build (see VRTs).

tdeekens

Thanks again. Had a second look:

I really like the addition of preval it makes a lot of sense
I wonder if we want to drop pug I don't see why we need it
We might use es6-template-strings - no direct benefit but avoids the __ replacements while we're here

As a note: We don't use JavaScript modules as type="module" yet. Support wise (not IE) we should. Still, it's to how I know and tell a change with Vite which might be worthwhile also documenting.

Lastly, I think the hardest part of understand in the vite setup is the asset and index.html re-routing but from what I can tell there is little we can do about it. I wonder just if it's fragile and could have bugs we don't know about yet.

packages/mc-dev-authentication/middlewares/create-mc-dev-authentication-middleware.js

tdeekens · 2022-04-27T09:11:32Z

packages/mc-dev-authentication/middlewares/create-mc-dev-authentication-middleware.js

+const pug = require('pug');
+const { logout } = require('../routes');
+
+const compileLoginView = pug.compileFile(


Not sure how you feel about it but using pug for these two feels a bit like uncalled-for complexity. For me personally, it doesn't ease anything. It's unrelated to the PR but I wounder if we could just remove pug? What are your thoughts?

I don't have a strong reason to keep using pug either. What would be the alternative? Plain old HTML?

tdeekens · 2022-04-27T09:15:02Z

packages/mc-html-template/html-docs/application.html

+    <!-- Fonts -->
+    <link href='https://fonts.googleapis.com/css?family=Open+Sans:300,300i,400,400i,600,600i,700,700i' rel='stylesheet' type='text/css'>
+
+    __APPLICATION_CSS_IMPORTS__


Also unrelated but given we started using es6-template-strings in other parts I wonder if it could make sense to use it too. Also an improvements we can make separately if we want to.

tdeekens · 2022-04-27T10:44:11Z

packages/mc-scripts/src/commands/build-vite.js

+      // Ensure to use the `cdnUrl` value when loading the entry point.
+      // NOTE: with Webpack this is done by setting the `__webpack_public_path__`.
+      const html = rawHtml.replace(
+        new RegExp(`<script type="module"(.*) src="(.*)">`, 'g'),


I guess alternatively we could parse the HTML which might be more expensive. Maybe defining function prependCdnUrlToScriptSrc would help. At least I got stuck here deducting that :)

emmenko · 2022-04-27T12:58:06Z

I really like the addition of preval it makes a lot of sense

Yeah that's kind of neat.

As a note: We don't use JavaScript modules as type="module" yet. Support wise (not IE) we should. Still, it's to how I know and tell a change with Vite which might be worthwhile also documenting.

You mean mentioning (changelog, etc.) that it uses <script type="module"?

Lastly, I think the hardest part of understand in the vite setup is the asset and index.html re-routing but from what I can tell there is little we can do about it. I wonder just if it's fragile and could have bugs we don't know about yet.

Yes unfortunately there is no built-in support for this (yet). I thought about actually dropping support for the cdnUrl property but it's still something useful to have I suppose.

The alternative is to set the config.base to point to the cdnUrl but it would mean no more "compile-html" step, so you need to rebuild the app every time something in the config changes.

tdeekens · 2022-04-27T13:20:04Z

You mean mentioning (changelog, etc.) that it uses <script type="module"?

Yes. Just so people are aware. I don't think app-kit has a strict browser policy at least I am not aware of any. However, all existing script tags are not of type=module to my knowledge and with using Vite would become unless I am mistaken.

Yes unfortunately there is no built-in support for this (vitejs/vite#3522 (comment)). I thought about actually dropping support for the cdnUrl property but it's still something useful to have I suppose.

I think it makes sense to keep the status quo even if that means the internals of the commands are a bit wonky. As a later step we could discuss removing some of that functionality if possible if we want to simplify those internals then.

emmenko · 2022-04-27T13:27:22Z

Actually there is an issue with the setup for the cdnUrl, as I had to set the config.base to '' (empty string) but this is causing other issues (see VRTs).

So I'll have to rethink how to handle that 🧐

…for cdnUrl

emmenko · 2022-04-27T13:53:44Z

So it seems that this is more tricky than I thought. The good news is that there is a general consensus on supporting this feature (configuring the base path on runtime). You can follow a bit the thread here: vitejs/vite#7611 (comment)

So for now, I would keep the setup simpler and avoid using the cdnUrl. This means that when using Vite, all assets must be served relative to the application index.html.
A warning will be logged to mention this.

tdeekens · 2022-04-27T14:03:37Z

So for now, I would keep the setup simpler and avoid using the cdnUrl. This means that when using Vite, all assets must be served relative to the application index.html.

I think that's a reasonable restriction then until Vite supports it natively.

…Url) and improve changeset

emmenko · 2022-04-27T14:41:30Z

Alright, see changes in the last commit (e2396e1) with the fixes and improvements.

tdeekens

Thanks. The simplification really makes it easier to follow. I think it's best to give it a shot also internally on small apps first and see how it behaves.

packages/mc-dev-authentication/middlewares/create-mc-dev-authentication-middleware.js

CarlosCortizasCT · 2022-04-28T08:38:05Z

packages/mc-scripts/src/bin/cli.js

+          console.log('Experimental Vite bundler enabled! 🚀');
+          console.warn(
+            'NOTE that the "cdnURL" value is not supported at the moment when using Vite.'
+          );


CarlosCortizasCT

🚀

emmenko added 🏕 Type: Technical Exploration Try out a new library or approach 🛠 Type: Tooling Things related to development tools for the repository labels Apr 25, 2022

tdeekens reviewed Apr 25, 2022

View reviewed changes

emmenko marked this pull request as ready for review April 26, 2022 15:12

emmenko force-pushed the nm-vitejs-experimental-bundler branch from dc15710 to 33ffaef Compare April 26, 2022 15:13

emmenko commented Apr 26, 2022

View reviewed changes

packages/mc-html-template/src/generate-template.js

);

return `

<!DOCTYPE html>

Copy link

Member Author

emmenko Apr 26, 2022

Choose a reason for hiding this comment

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

Move to a separate HTML file.

vercel bot temporarily deployed to Preview April 26, 2022 15:18 Inactive

emmenko commented Apr 26, 2022

View reviewed changes

emmenko requested review from tdeekens and a team April 26, 2022 15:25

vercel bot temporarily deployed to Preview April 26, 2022 15:27 Inactive

tdeekens reviewed Apr 26, 2022

View reviewed changes

emmenko marked this pull request as draft April 27, 2022 10:29

tdeekens reviewed Apr 27, 2022

View reviewed changes

emmenko added 9 commits April 27, 2022 15:27

refactor: use vite

4774f85

feat(mc-scripts): add experimental support for Vite bundler

6b5e394

refactor: use experimental bundler in playground

96a914d

docs: changesets

aa7ff94

fix: use preval macro for preloading static file on build

5d3b02a

chore: rename env var

3379f21

fix(mc-scripts): dynamically change public path of assets on runtime …

50ea233

…for cdnUrl

fix(mc-scripts): revert unnecessary change

cb0bd5f

refactor: simplify logic

9bdff47

refactor(mc-scripts): drop support for dynamic runtime base path (cdn…

e2396e1

…Url) and improve changeset

emmenko force-pushed the nm-vitejs-experimental-bundler branch from 2ecc035 to e2396e1 Compare April 27, 2022 14:40

emmenko marked this pull request as ready for review April 27, 2022 14:41

vercel bot temporarily deployed to Preview April 27, 2022 14:45 Inactive

tdeekens approved these changes Apr 27, 2022

View reviewed changes

CarlosCortizasCT reviewed Apr 28, 2022

View reviewed changes

packages/mc-dev-authentication/middlewares/create-mc-dev-authentication-middleware.js Show resolved Hide resolved

CarlosCortizasCT reviewed Apr 28, 2022

View reviewed changes

CarlosCortizasCT approved these changes Apr 28, 2022

View reviewed changes

emmenko merged commit 5bcf106 into main Apr 28, 2022

emmenko deleted the nm-vitejs-experimental-bundler branch April 28, 2022 12:36

ghost mentioned this pull request Apr 28, 2022

Version Packages #2564

Merged

emmenko mentioned this pull request Apr 29, 2022

Vite support: missing features #2577

Open

6 tasks

emmenko mentioned this pull request May 27, 2022

fix(mc-scripts): local login dev for internal usage #2626

Merged

	Enable opt-in support for experimental bundler (using [Vite.js](https://vitejs.dev/)). To enable it, set the environment variable `ENABLE_EXPERIMENTAL_BUNDLER="true"` in your dotenv file.
	Enable opt-in support for experimental bundler (using [Vite.js](https://vitejs.dev/)). To enable it, set the environment variable `ENABLE_EXPERIMENTAL_VITE_BUNDLING=true"` in your dotenv file.

		const htmlLogin = compileLoginView({ env: applicationConfig.env });
		const htmlLogout = compileLogoutView({ env: applicationConfig.env });

		const createLoginMiddleware = require('./create-login-middleware');
		const createLogoutMiddleware = require('./create-logout-middleware');

		allowedHost: urls.localUrlForBrowser,
		contentBase: paths.appBuild,

		@@ -0,0 +1,11 @@
		window.__dynamicImportHandler__ = function (importer) {

feat(cli): add opt-in experimental support for Vite bundler #2568

feat(cli): add opt-in experimental support for Vite bundler #2568

Conversation

emmenko commented Apr 25, 2022 • edited

changeset-bot bot commented Apr 25, 2022 • edited

🦋 Changeset detected

vercel bot commented Apr 25, 2022 • edited

tdeekens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmenko commented Apr 25, 2022

tdeekens commented Apr 26, 2022

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

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

tdeekens 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmenko commented Apr 27, 2022 • edited

tdeekens left a comment • edited

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

emmenko commented Apr 27, 2022

tdeekens commented Apr 27, 2022

emmenko commented Apr 27, 2022

emmenko commented Apr 27, 2022

tdeekens commented Apr 27, 2022

emmenko commented Apr 27, 2022

tdeekens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosCortizasCT left a comment

Choose a reason for hiding this comment

emmenko commented Apr 25, 2022 •

edited

changeset-bot bot commented Apr 25, 2022 •

edited

vercel bot commented Apr 25, 2022 •

edited

emmenko commented Apr 27, 2022 •

edited

tdeekens left a comment •

edited