Enable Embroider/Webpack code spliting for Wizard #23678

chancancode · 2023-09-27T01:19:44Z

This PR includes a lot of refactors and cleanups that can stand on their own/works in the current build. For your reviewing pleasure, you can view these chunks separately:

Refactor Wizard models (Refactor Wizard models #23640)
Refactor Wizard routes/controllers/templates (PR pending once Refactor Wizard models #23640 is merged)
Refactor Wizard components (PR pending once Refactor Wizard models #23640 and the above are merged)
Enable Embroider/Webpack code splitting for Wizard (PR pending once Refactor Wizard models #23640 and the above are merged)

I do recommend reviewing the commits individually, since the move/rename to gjs lost track of some of the diff

To verify, you can run this branch locally, go to any non-wizard route, inspect the loaded chunks to see that none of the wizard code is loaded (other than the stub for the route entrypoint), and then navigate to /wizard to see that it loaded an additional chunk with all the wizard code in there.

Move Wizard back into main app, remove Wizard addon
Remove Wizard-related resolver or build hacks
Add a app/static folder in the app, add it to staticAppPaths
Install and enable @embroider/router
Add "wizard" to splitAtRoutes

In a fully optimized Embroider app, route-based code splitting more or less Just Work™ – install @embroider/router, subclass from it, configure which routes you want to split and that's about it.

However, our app is not "fully optimized", by which I mean we are not able to turn on all the static* flags.

In Embroider, "static" means "statically analyzable". Specifically it means that all inter-dependencies between modules (files) are explicitly expressed as imports, as opposed to {{i18n ...}} magically means "look for the default export in app/helpers/i18n.js" or something even more dynamic with the resolver.

Without turning on those flags, Embroider behaves conservatively, slurps up all app files eagerly into the primary bundle/chunks. So, while you could turn on route-based code splitting, there won't be much to split.

The commits leading up to this involves a bunch of refactors and cleanups that 1) works perfectly fine in the classic build, 2) are good and useful in their own right, but also 3) re-arranged things such that most dependencies are now explicit.

With those in place, I was able to move all the wizard code into the "app/static" folder. Embrodier does not eagerly pull things from this folder into any bundle, unless something explicitly "asks" for them via imports. Conversely, thigns from this folder are not registered with the resolver and are not added to the loader.js registry.

In conjunction with route-based code splitting, we now have the ability to split out islands of on-demand functionalities from the main app bundle.

When you split a route in Embroider, it automatically creates a bundle/entrypoint with the relevant routes/templates/controllers matching that route prefix. Anything they import will be added to the bundle as well, assuming they are not already in the main app bundle, which is where the "app/static" folder comes into play.

The "app/static" folder name is not special. It is configured in ember-cli-build.js. Alternatively, we could have left everything in their normal locations, and add more fine-grained paths to the staticAppPaths array. I just thought it would be easy to manage and scale, and less error-prone to do it this way.

Note that putting things in app/static does not guarantee that it would not be part of the main app bundle. For example, if we were to add an import ... from "app/static/wizard/..."; in a main bundle file (say, app.js), then that chunk of the module graph would be pulled in. (Consider using await import(...)?)

Overtime, we can build better tooling (e.g. lint rules and babel macros to make things less repetitive) as we expand the use of this pattern, but this is a start.

chancancode · 2023-09-27T06:13:55Z

Lint failure will be fixed by ember-template-lint/ember-template-lint#2982, or we can fix it in our own config

NullVoxPopuli · 2023-09-27T11:50:42Z

Yaaaaas! Excitement!

app/assets/javascripts/discourse/app/routes/app-route-map.js

app/assets/javascripts/discourse/app/routes/wizard.js

app/assets/javascripts/discourse/app/routes/wizard/step.js

chancancode · 2023-09-27T16:27:58Z

app/assets/javascripts/discourse/app/static/wizard/components/fields/components/index.js

+
+export default {
+  "styling-preview": StylingPreview,
+};


Since this is the only thing left, we may want to consider getting rid of the type: "component" infrastructure and just do type: "styling-preview"?

chancancode · 2023-09-27T16:28:30Z

...ets/javascripts/discourse/app/static/wizard/components/fields/components/styling-preview.hbs

@@ -8,7 +8,7 @@
    </canvas>
  </div>
  <div class="wizard-container__preview homepage-preview">
-    <HomepagePreview @wizard={{this.wizard}} @step={{this.step}} />
+    <this.HomepagePreview @wizard={{this.wizard}} @step={{this.step}} />


@davidtaylorhq this is what I mentioned on the call

chancancode · 2023-09-27T16:28:48Z

...sets/javascripts/discourse/app/static/wizard/components/fields/components/styling-preview.js

@@ -19,15 +20,20 @@ export default WizardPreviewBaseComponent.extend({
  draggingActive: false,
  startX: 0,
  scrollLeft: 0,
+  HomepagePreview,


@davidtaylorhq counterpart of the above

chancancode · 2023-09-27T16:31:49Z

app/assets/javascripts/discourse/app/static/wizard/components/fields/dropdown.hbs

I suppose this could just use the <this.component> syntax too

chancancode · 2023-09-27T16:35:18Z

app/assets/javascripts/discourse/app/static/wizard/components/wizard-field.gjs

+
+  get fieldClass() {
+    return `field-${dasherize(this.field.id)} wizard-focusable`;
+  }


So this is is interesting. It seems like the fields are expected to put the @fieldClass on their element, but very few actually do, and this is almost dead code. And those that do use it, does not put it on an actual "focusable" DOM element. This class is in turns used by the autoFocus code in the wizard. So, basically the "autofocus" code doesn't really do anything right now. But I kept it around (not) working exactly as it was before to let someone else tackle that after.

chancancode · 2023-09-27T16:35:37Z

app/assets/javascripts/discourse/app/static/wizard/components/wizard-step.gjs

+
+      $(".wizard-focusable:nth-of-type(1)").focus();
+    });
+  }


See comment above: this doesn't really do anything.

chancancode · 2023-09-27T16:37:23Z

app/assets/javascripts/discourse/app/static/wizard/components/wizard-step.gjs

+    Jump In: on the "ready" page, it exits the setup ("finish"), on the
+    last page, it saves, and if successful, go to the home page
+
+   */


This is actually a considerable improvement over the status quo, in that the only step that we sort of hardcode is "ready". It will work just fine if that step isn't present, but if it does, it splits the wizard flow into the "required" and "optional" steps and displays the buttons correctly, without hard coding in "styling", "branding", etc. And the final step is now only "special" by way of being last, rather than hard coding "corporate".

chancancode · 2023-09-27T16:41:05Z

app/assets/javascripts/discourse/app/static/wizard/components/wizard-step.gjs

+    } finally {
+      this.saving = false;
+    }
+  }


This doesn't exactly do what you would imagine (but is ported over exactly as-is). You would think the try/catch is there to guard the await this.step.save() step failing, but that step actually never fails, because it has a catch that never re-throws the error.

What actually happens is that if an error happens in this.step.save(), it will fail to return a response value (response === undefined), and the code in @goNext is guarded with response?.blah such that it doesn't actually do anything if response is undefined.

I don't think that is how anyone is envisioning things to work 😬

Probably should make step.save re-throw, so that we never call this.args.goNext when the save fails. But I left it for someone else to tackle.

chancancode · 2023-09-27T16:44:52Z

app/assets/javascripts/discourse/app/templates/wizard/step.gjs

+        this.router.transitionTo("wizard.step", next);
+      } else if (response?.success) {
+        this.router.transitionTo("discovery.latest");
+      }


This is the counterpart of the comment above. If response is undefined, this doesn't end up doing anything

chancancode · 2023-09-27T16:45:37Z

app/assets/javascripts/discourse/tests/acceptance/sidebar-user-test.js

@@ -7,6 +7,7 @@ import {
  query,
  updateCurrentUser,
 } from "discourse/tests/helpers/qunit-helpers";
+import "discourse/routes/wizard";


Because the wizard route is not guaranteed to be in the main bundle, tests that want to have it loaded needs to do this.

chancancode · 2023-09-27T16:46:25Z

app/assets/javascripts/discourse/tests/helpers/wizard-pretender.js

@@ -20,24 +20,47 @@ export default function (helpers) {
                description: "Your name",
              },
            ],
-            next: "styling",
+            next: "hello-again",


Beefed up the test to have all five possibilities in the table

chancancode · 2023-09-27T16:50:00Z

app/assets/javascripts/discourse/app/routes/wizard.js

@@ -1,31 +1,31 @@
 import Route from "@ember/routing/route";


Note that this file lives in the normal location (app/routes/wizard.js). With splitAtRoute: ["wizard"], app/routes/wizard, app/routes/wizard/*, app/controllers/wizard, app/controllers/wizard/*, app/templates/wizard, app/templates/wizard/* all get split off into the lazy bundle. In an earlier iteration, I experimented with putting all of these files primarily in app/static/wizard, and added a stub to re-export them in the normal location, but that ended up being a lot of extra boilerplate IMO.

chancancode · 2023-09-27T16:50:42Z

app/assets/javascripts/discourse/app/static/wizard/components/fields/component.gjs

+
+  get component() {
+    let id = dasherize(this.args.field.id);
+    assert(`"${id}" is not a valid wizard component`, id in components);


We could supplement this with a resolver lookup as a fallback if necessary

chancancode · 2023-09-27T16:53:04Z

app/assets/javascripts/discourse/app/static/wizard/models/wizard.js

+    } catch (error) {
+      for (let err of error.jqXHR.responseJSON.errors) {
+        this.fieldError(err.field, err.description);
+      }


This is where it doesn't re-throw (or throw a new error)

chancancode · 2023-09-27T16:54:16Z

app/assets/javascripts/discourse/app/templates/wizard.gjs

+    <div class="discourse-logo">
+      <DiscourseLogo />
+    </div>
+


I moved the canvas into the step, which works just as well (since it's position: fixed anyway), but avoids having having access to the child route's step here somehow, which was a bit awkward

chancancode · 2023-09-27T16:56:15Z

app/assets/javascripts/discourse/app/templates/wizard/step.gjs

+    <template>
+      {{#if this.showCanvas}}
+        <WizardCanvas />
+      {{/if}}


We may even want to move this into WizardStep itself. It probably wan't possibly previously because the wrapping div was used for styling, but since I migrated it to glimmer component it should be no problem now

chancancode · 2023-09-27T17:44:52Z

app/assets/stylesheets/wizard.scss

-    @include breakpoint("mobile-extra-large") {
-      order: 1;
-      margin-left: 0;
-    }
    &:hover {
      background-color: var(--primary-300);
    }


I don't actually think this is intentional, but I kept it here.

Previously, the "Jump In" button did not have the .primary CSS class, but it very much is and wants to be the primary action, and duplicated a bunch of styles from .primary. Now, I just added the primary class to the "Jump In" button and removed the redundant rules. This is the only difference left and it makes the hover shade a bit different than the "Next" button. To my eyes it looked off and I think we probably just want to remove this, but I left it as is for now.

This is extracted from discourse#23678, look there for a longer description

This is extracted from #23678, look there for a longer description

+ native classes + tracked properties - Ember.Object - Ember.Evented - observers - mixins - computed/discourseComputed Also removes unused wizard infrastructure for warnings. It appears that once upon on time, either the server can generate warnings, or some client code can generate them, which requires an extra confirmation from the user before they can continue to the next step. This code is not tested and appears unused and defunct. Nothing generates such warning and the server does not serialize them. Extracted from #23678

This code has gone through a lot of refactors and repurposing, and things have become more complicated and confusing than they needed to be. At present, the Wizard goes through a few "required" steps for the basics, then shows a "ready" screen directing the user to "jump in" but have an option to continue on for optional screens, eventually ending on the "corporate" screen which is the final step. On desktop, there are two groups of buttons – 1. on the left side, it shows a back button except for the first screen 2. on the right side, there is a always a primary button (either "next" or "jump in"), and then to its left, there is sometimes a secondary action styled as a link (either "configure more" or "exit setup") On mobile, these buttons are stacked, with the back button sitting at the bottom, if present. In table form: Step Back Button? Primary Action Secondary Action ------------------------------------------------------------------ First No Next N/A ------------------------------------------------------------------ ... Yes Next N/A ------------------------------------------------------------------ Ready Yes Jump In Configure More ------------------------------------------------------------------ ... Yes Next Exit Setup ------------------------------------------------------------------ Last Yes Jump In N/A ------------------------------------------------------------------ Back Button: without saving, go back to the last page Next Button: save, and if successful, go to the next page Configure More: re-skinned next button Exit Setup: without saving, go to the home page ("finish") Jump In: on the "ready" page, it exits the setup ("finish"), on the last page, it saves, and if successful, go to the home page This commit re-aligns the code to reflect this reality without the unnecessary indirections leftover from previous refactors.

Move field components into subfolders, remove dependency on string resolution with the runtime resolver.

* Move Wizard back into main app, remove Wizard addon * Remove Wizard-related resolver or build hacks * Add a `app/static` folder in the app, add it to `staticAppPaths` * Install and enable `@embroider/router` * Add "wizard" to `splitAtRoutes` In a fully optimized Embroider app, route-based code splitting more or less Just Work™ – install `@embroider/router`, subclass from it, configure which routes you want to split and that's about it. However, our app is not "fully optimized", by which I mean we are not able to turn on all the `static*` flags. In Embroider, "static" means "statically analyzable". Specifically it means that all inter-dependencies between modules (files) are explicitly expressed as `import`s, as opposed to `{{i18n ...}}` magically means "look for the default export in app/helpers/i18n.js" or something even more dynamic with the resolver. Without turning on those flags, Embroider behaves conservatively, slurps up all `app` files eagerly into the primary bundle/chunks. So, while you _could_ turn on route-based code splitting, there won't be much to split. The commits leading up to this involves a bunch of refactors and cleanups that 1) works perfectly fine in the classic build, 2) are good and useful in their own right, but also 3) re-arranged thigns such that most dependencies are now explicit. With those in place, I was able to move all the wizard code into the "app/static" folder. Embrodier does not eagerly pull things from this folder into any bundle, unless something explicitly "asks" for them via `imports`. Conversely, thigns from this folder are not registered with the resolver and are not added to the `loader.js` registry. In conjunction with route-based code splitting, we now have the ability to split out islands of on-demand functionalities from the main app bundle. When you split a route in Embroider, it automatically creates a bundle/entrypoint with the relevant routes/templates/controllers matching that route prefix. Anything they import will be added to the bundle as well, assuming they are not alreay in the main app bundle, which is where the "app/static" folder comes into play. The "app/static" folder name is not special. It is configured in ember-cli-build.js. Alternatively, we could have left everything in their normal locations, and add more fine-grained paths to the `staticAppPaths` array. I just thought it would be easy to manage and scale, and less error-prone to do it this way. Note that putting things in `app/static` does not guarentee that it would not be part of the main app bundle. For example, if we were to add an `import ... from "app/static/wizard/...";` in a main bundle file (say, `app.js`), then that chunk of the module graph would be pulled in. (Consider using `await import(...)`?) Overtime, we can build better tooling (e.g. lint rules and babel macros to make things less repetitive) as we expand the use of this pattern, but this is a start.

Extracted from #23678 Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>

This commit refactors the Wizard component code in preparation for moving it to the 'static' directory for Embroider route-splitting. It also includes a number of general improvements and simplifications. Extracted from #23678 Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>

(extracted from #23678) * Move Wizard back into main app, remove Wizard addon * Remove Wizard-related resolver or build hacks * Install and enable `@embroider/router` * Add "wizard" to `splitAtRoutes` In a fully optimized Embroider app, route-based code splitting more or less Just Work™ – install `@embroider/router`, subclass from it, configure which routes you want to split and that's about it. However, our app is not "fully optimized", by which I mean we are not able to turn on all the `static*` flags. In Embroider, "static" means "statically analyzable". Specifically it means that all inter-dependencies between modules (files) are explicitly expressed as `import`s, as opposed to `{{i18n ...}}` magically means "look for the default export in app/helpers/i18n.js" or something even more dynamic with the resolver. Without turning on those flags, Embroider behaves conservatively, slurps up all `app` files eagerly into the primary bundle/chunks. So, while you _could_ turn on route-based code splitting, there won't be much to split. The commits leading up to this involves a bunch of refactors and cleanups that 1) works perfectly fine in the classic build, 2) are good and useful in their own right, but also 3) re-arranged things such that most dependencies are now explicit. With those in place, I was able to move all the wizard code into the "app/static" folder. Embroider does not eagerly pull things from this folder into any bundle, unless something explicitly "asks" for them via `imports`. Conversely, things from this folder are not registered with the resolver and are not added to the `loader.js` registry. In conjunction with route-based code splitting, we now have the ability to split out islands of on-demand functionalities from the main app bundle. When you split a route in Embroider, it automatically creates a bundle/entrypoint with the relevant routes/templates/controllers matching that route prefix. Anything they import will be added to the bundle as well, assuming they are not already in the main app bundle, which is where the "app/static" folder comes into play. The "app/static" folder name is not special. It is configured in ember-cli-build.js. Alternatively, we could have left everything in their normal locations, and add more fine-grained paths to the `staticAppPaths` array. I just thought it would be easy to manage and scale, and less error-prone to do it this way. Note that putting things in `app/static` does not guarantee that it would not be part of the main app bundle. For example, if we were to add an `import ... from "app/static/wizard/...";` in a main bundle file (say, `app.js`), then that chunk of the module graph would be pulled in. (Consider using `await import(...)`?) Overtime, we can build better tooling (e.g. lint rules and babel macros to make things less repetitive) as we expand the use of this pattern, but this is a start. Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>

davidtaylorhq · 2023-12-22T13:34:29Z

Final part merged via cbc28e8

chancancode commented Sep 27, 2023

View reviewed changes

app/assets/javascripts/discourse/app/routes/app-route-map.js Show resolved Hide resolved

chancancode commented Sep 27, 2023

View reviewed changes

app/assets/javascripts/discourse/app/routes/wizard.js Show resolved Hide resolved

chancancode commented Sep 27, 2023

View reviewed changes

app/assets/javascripts/discourse/app/routes/wizard/step.js Show resolved Hide resolved

chancancode commented Sep 27, 2023

View reviewed changes

davidtaylorhq added the js-modernization Tasks related to our JS modernization project, including the Ember 4.0+ upgrade label Sep 28, 2023

chancancode force-pushed the embroider-wizardry branch from 2bb3bbb to dbe71dd Compare October 4, 2023 14:27

chancancode added a commit to chancancode/discourse that referenced this pull request Oct 12, 2023

Introduce the app/static folder

46e108e

This is extracted from discourse#23678, look there for a longer description

chancancode added a commit to chancancode/discourse that referenced this pull request Oct 12, 2023

Introduce the app/static folder

20fef28

This is extracted from discourse#23678, look there for a longer description

chancancode added a commit to chancancode/discourse that referenced this pull request Oct 19, 2023

Introduce the app/static folder

025eb5f

This is extracted from discourse#23678, look there for a longer description

chancancode force-pushed the embroider-wizardry branch from dbe71dd to e92eb03 Compare October 19, 2023 23:30

chancancode force-pushed the embroider-wizardry branch 2 times, most recently from 0a062a7 to 0f55e63 Compare October 20, 2023 04:05

davidtaylorhq pushed a commit that referenced this pull request Oct 26, 2023

Introduce the app/static folder

31f9ef3

This is extracted from #23678, look there for a longer description

chancancode added 10 commits November 23, 2023 16:42

Enable .gjs on wizard

c88a74e

Refactor route templates to gjs

db90e00

Refactor Wizard routes

e29f1ab

Deeper refactor, removing controllers

3d5e4ec

Move quit action to route component

2055c36

Refactor <WizardStep> to .gjs

4000710

Refactor <WizardCanvas> to .gjs

a7dede8

Refactor <WizardField> to .gjs

7cf7500

Move field components into subfolders, remove dependency on string resolution with the runtime resolver.

Remove <WizardStepForm>

0366e4f

davidtaylorhq force-pushed the embroider-wizardry branch from 0f55e63 to 60cc6b0 Compare November 23, 2023 16:49

davidtaylorhq force-pushed the embroider-wizardry branch from 60cc6b0 to 42ef186 Compare November 23, 2023 17:02

davidtaylorhq mentioned this pull request Dec 5, 2023

DEV: Refactor Wizard routes, controllers and templates #24725

Merged

davidtaylorhq added a commit that referenced this pull request Dec 6, 2023

DEV: Refactor Wizard routes, controllers and templates (#24725)

b3d1707

Extracted from #23678 Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>

carsick pushed a commit that referenced this pull request Dec 6, 2023

DEV: Refactor Wizard routes, controllers and templates (#24725)

0abd7c3

Extracted from #23678 Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>

davidtaylorhq mentioned this pull request Dec 7, 2023

DEV: Refactor Wizard components #24770

Merged

davidtaylorhq mentioned this pull request Dec 15, 2023

Enable Embroider/Webpack code spliting for Wizard #24919

Merged

davidtaylorhq closed this Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Embroider/Webpack code spliting for Wizard #23678

Enable Embroider/Webpack code spliting for Wizard #23678

chancancode commented Sep 27, 2023 •

edited

chancancode commented Sep 27, 2023

NullVoxPopuli commented Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023

chancancode Sep 27, 2023 •

edited

chancancode Sep 27, 2023

chancancode Sep 27, 2023

davidtaylorhq commented Dec 22, 2023

Enable Embroider/Webpack code spliting for Wizard #23678

Enable Embroider/Webpack code spliting for Wizard #23678

Conversation

chancancode commented Sep 27, 2023 • edited

chancancode commented Sep 27, 2023

NullVoxPopuli commented Sep 27, 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

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

chancancode Sep 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtaylorhq commented Dec 22, 2023

chancancode commented Sep 27, 2023 •

edited

chancancode Sep 27, 2023 •

edited