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

Lazy loading of Stimulus controllers is different from stimulus-bridge #87

Open
YummYume opened this issue Apr 22, 2024 · 4 comments
Open
Labels
bug Something isn't working

Comments

@YummYume
Copy link

vite-bundle version

6.4.4

vite-plugin-symfony version

6.4.3

your OS, Symfony version, PHP version, Node version

Linux, Symfony 7.0.6, PHP 8.3.6, Node 20.11.0

Description

Hello, first off, thanks a lot for making this bundle 🙏 It's incredible.

While migrating multiple apps from Webpack Encore to this bundle, I noticed that the lazy-loading of Stimulus controllers was not working the same way as when using stimulus-bridge.

The excepted behaviour was to only load controllers when they're actually used on the page. This is what was achieved using stimulus-bridge and, as the doc claims, "By default all your controllers are lazy loaded.". However, the lazy-loading of Vite still loads all the controllers, even when they're not actually used.

This isn't a problem for small apps, but for medium to large sized apps, or apps using external bundles, it can be quite troublesome to load so many controllers (and have so many http requests) on a page that would only use one or two controllers at most.

Is this the intended behaviour of this bundle ? Or am I missing something ?

How to reproduce

Add a few controllers in assets/controllers for example.

// assets/bootstrap.js
const app = startStimulusApp();

registerControllers(
  app,
  import.meta.glob('./controllers/*_(lazy)\?controller.[jt]s(x)\?') 
)

Then go to a page where you don't use any controllers and inspect the network. All the controller files are still being loaded.

Possible Solution

Check the stimulus-bridge implementation.

What happens here is that controllers marked as "lazy" are not actually registered, but being proxied by a generate-lazy-controller, which checks if any lazy loaded controller is present in the application, and only then loads them.

@YummYume YummYume added the bug Something isn't working label Apr 22, 2024
@YummYume
Copy link
Author

After investigating a bit, and looking at the source code, it seems that only controllers ending with lazycontroller.* will get properly registered as "lazy". The documentation about this is very confusing and led me to think that they would all be lazy-loaded by default unless I defined eager: true in the glob options.

I ended up making a function to change the name of my controllers to satisfy this constraint, like so :

const transformImportsToLazy = (imports: Record<string, () => Promise<unknown>>) => {
  const lazyImports: Record<string, () => Promise<unknown>> = {};

  for (const [key, value] of Object.entries(imports)) {
    if (key.match(/_controller\.(ts|js)$/)) {
      lazyImports[key.replace(/_controller\.(ts|js)$/, '_lazycontroller.$1')] = value;
    } else {
      lazyImports[key] = value;
    }
  }

  return lazyImports;
};

Maybe the documentation could be updated to better explain this part ?

@lhapaipai
Copy link
Owner

lhapaipai commented May 9, 2024

I @YummYume,
indeed there are some differences between the 2 implementations due to the fact that
webpack uses commonjs

// with webpack-encore
export const app = startStimulusApp(require.context(
    '@symfony/stimulus-bridge/lazy-controller-loader!./controllers',
    true,
    /\.[jt]sx?$/
));

and Vite the ES Modules

// with vite
const app = startStimulusApp();
registerControllers(
  app,
  import.meta.glob('./controllers/*_(lazy)\?controller.[jt]s(x)\?')
)

the glob import is lazy by default : https://vitejs.dev/guide/features#glob-import

here is the current behavior

const modules = import.meta.glob('...', { eager: true })
/*
modules === {
  "./controllers/notifier_controller.js": {
    default: <your-stimulus-controller>
  },
  // ...
}
*/
const lazyModules = import.meta.glob('...');
/*
modules === {
  "./controllers/notifier_controller.js": () => Promise.resolve({
    default: <your-stimulus-controller>
  }),
  // ...
}
*/

if you do an import with import.meta.glob('....', { eager: true }) when you call registerControllers your files are already loaded, it is too late to have lazy behavior.
So in this case, at build time, your entry point and all your controllers will be compiled into a single file.

on the other hand if you do an import with import.meta.glob('....') when you call registerControllers your files are not yet loaded.
So in this case, at build time, your entry point and all your controllers will be compiled into separate files.
However, at this time we still have not decided on the behavior to follow when calling the registerControllers function.

The behavior will be dictated by the file name suffix. if it ends with:

  • _controller.[jt]s(x): the function is called, the promise is resolved and we will have an operation similar to the eager behavior. The import is resolved when the script is executed
  • _lazycontroller.[jt]s(x): our controller is wrapped in a lazyController which will only resolve the promise when the controller is mounted the first time. The import is only resolved when a DOM element is present for the first time which requests to load the controller.

actually the doc is incorrect compared to my explanation... Before updating it can you give me your opinion. Do you achieve the desired behavior with my explanations? Do you have any suggestions? Could you explain to me in more detail in what context you use your transformImportsToLazy function described above?

here the function where all the mechanics do their work, but I think you already know that...

// https://github.com/lhapaipai/symfony-vite-dev/blob/ef656db6eaf49d50fcdda62d131a55ec9a307724/src/vite-plugin-symfony/src/stimulus/helpers/base.ts#L49

type LazyModule<M> = () => Promise<M>;
type ImportedModule<M> = M | LazyModule<M>;
type ImportedModules<M> = Record<string, ImportedModule<M>>;

export function registerControllers(app: Application, modules: ImportedModules<ControllerModule>) {
  Object.entries(modules).forEach(([filePath, importedModule]) => {
    const { identifier, lazy } = getStimulusControllerFileInfos(filePath);
    if (!identifier) {
      throw new Error(`Invalid filePath name ${filePath}`);
    }

    
    if (typeof importedModule === "function") {
      // modules loaded with import.meta.glob('....', { eager: false })
      if (lazy) {
        app.register(identifier, getLazyController(importedModule));
      } else {
        importedModule().then((controllerConstructor) => {
          if (identifier && typeof controllerConstructor.default === "function") {
            app.register(identifier, controllerConstructor.default);
          }
        });
      }
    } else {
      // modules loaded with import.meta.glob('....', { eager: true })
      app.register(identifier, importedModule.default);
    }
  });
}

good afternoon

@lhapaipai
Copy link
Owner

lhapaipai commented May 9, 2024

to complete the discussion you can combine all this imports...

with 4 controllers for example

.
└── assets
    ├── controllers
    │   ├── counter_optional_lazycontroller.js
    │   ├── message_optional_controller.js
    │   ├── notifier_controller.js
    │   └── welcome_core_controller.js
    ├── bootstrap.js
    └── controllers.json
import {
  startStimulusApp,
  registerControllers,
} from "vite-plugin-symfony/stimulus/helpers";

import notifier from "./controllers/notifier_controller";

const app = startStimulusApp();

registerControllers(app, {
  ...import.meta.glob('./controllers/*_optional_(lazy)\?controller.[jt]s(x)\?'),
  ...import.meta.glob('./controllers/*_core_(lazy)\?controller.[jt]s(x)\?', { eager: true }),
})

// basic sync registration
app.register("notifier", notifier);

the name of your controller will be the name of your file without _controller.xxx / _lazycontroller.xxx.
and with _ replaced by -.

example:

  • counter_optional_lazycontroller.js -> counter-optional

it is possible to adapt the regex : ./controllers/*_(lazy)\?controller.[jt]s(x)\? but there is some contraints.
check the source file and the test file to get all the cases

finally one last piece of information regarding the stimulus debug mode

the normal life cycle of an eager or dynamic component not wrapped in a lazy controller is

.
├── welcome #initialize
├── welcome #connect
└── welcome #disconnect

for a dynamic component that has been wrapped in a lazy controller it will be:

.
├── welcome #initialize     (wrapper) -> begin download of the real controller
├── welcome #connect        (wrapper) -> very short lifecycle until the real controller is fully downloaded
└── welcome #disconnect     (wrapper) -> we disconnect the wrapper so that it is replaced by the real controller
    ├── welcome #initialize (real controller)
    ├── welcome #connect    (real controller)
    └── welcome #disconnect (real controller)

the documentation definitely needs to be updated but I'm waiting for your feedback before tackling it

@YummYume
Copy link
Author

Hi @lhapaipai, thanks for such a detailed answer !

Back when we were using Webpack Encore, all our controller files ended with _controller.{ts,js}. We had to register multiple controller directories coming from different places, where most of the controllers had the magic comment /* stimulusFetch: 'lazy' */ on them.

However, sometimes, we needed some controllers to not be lazy-loaded, depending on the app. Because, for example, some controllers needed to be connected as soon as possible to listen to events, or be registered as outlets. In this case, we would manually register the controller again, but without the lazy-loading behaviour. Here's how it looked like :

import { definitionsFromContext } from '@hotwired/stimulus-webpack-helpers';
import { startStimulusApp } from '@symfony/stimulus-bridge';
import XController from './controllers/x_controller';

export const app = startStimulusApp(
  require.context('@symfony/stimulus-bridge/lazy-controller-loader!./controllers', true, /\.[jt]sx?$/),
);

app.load(
  definitionsFromContext(
    require.context('@symfony/stimulus-bridge/lazy-controller-loader!$first-alias/controllers', true, /\.[jt]sx?$/),
  ),
);

app.load(
  definitionsFromContext(
    require.context('@symfony/stimulus-bridge/lazy-controller-loader!$second-alias/controllers', true, /\.[jt]sx?$/),
  ),
);

// This controller has already been registered, but we need it non-lazy for this app
app.register('x-controller', XController);

Now, after migrating to Vite, the controllers would be lazy-loaded as you stated above, but they wouldn't be wrapped in the special lazy controller. This was because they were all ending with _controller.{ts,js} and not _lazycontroller.{ts,js}, which is why I created this transformImportsToLazy function to fake their real name to get the real lazy-loading behaviour. Here's the full implementation :

export const transformImportsToLazy = (
  imports: Record<string, () => Promise<unknown>>,
  exclude: string[] = [],
  controllerMatch = /_controller\.(ts|js)$/,
) => {
  const lazyImports: Record<string, () => Promise<unknown>> = {};

  for (const [key, value] of Object.entries(imports)) {
    if (key.match(controllerMatch) && !exclude.includes(key)) {
      lazyImports[key.replace(controllerMatch, '_lazycontroller.$1')] = value;
    } else {
      lazyImports[key] = value;
    }
  }

  return lazyImports;
};

And here's how the app looks like with Vite currently :

import { registerControllers, startStimulusApp } from 'vite-plugin-symfony/stimulus/helpers';

import { transformImportsToLazy } from './vite';

export const app = startStimulusApp();

app.debug = import.meta.env.MODE === 'development';

registerControllers(
  app,
  // @ts-expect-error - controllers are lazy loaded
  transformImportsToLazy(import.meta.glob('./controllers/**/*_(lazy)?controller.[jt]s(x)?'), [
    // This controller should not be lazy-loaded afterall, so we do not convert its name
    './controllers/x_controller.ts',
  ]),
);

registerControllers(
  app,
  // @ts-expect-error - controllers are lazy loaded
  transformImportsToLazy(import.meta.glob('$first-alias/controllers/**/*_(lazy)?controller.[jt]s(x)?')),
);

registerControllers(
  app,
  // @ts-expect-error - controllers are lazy loaded
  transformImportsToLazy(import.meta.glob('$second-alias/controllers/**/*_(lazy)?controller.[jt]s(x)?')),
);

Notice the @ts-expect-error, because the typing seems to be wrong and only accept eager imports (without a Promise).

Now, this implementation works fine, but is merely a workaround. Having to manually set the names of the controllers we do not want to have lazy loaded is also not ideal (if they change name, etc...). And excluding their name from the glob would quickly create a messy, hard to read string.

I think the solution could be one of many, but the most important would probably be to either not use the name to match "lazy" controllers, or at least give control for the logic used. It could also be convenient if controllers could directly declare their default behaviour (lazy or not) inside their file directly. It could make the migration from Webpack to Vite easier for larger apps. But I don't know how hard that would be to implement. What do you think ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants