flarum / issue-archive

0 stars 0 forks source link

Unify javascript imports, exports and compat from core and extensions #165

Open clarkwinkelmann opened 3 years ago

clarkwinkelmann commented 3 years ago

This is not a new subject but I don't think we have a GitHub issue specific for it, so here it is.

Right now, we use two different systems for internal import/export inside a single extension, and external import/export between extensions.

Internal imports use the built-in ES module loader from webpack.

External imports use two different methods:

There are various problems with those approaches:

I have researched possible improvements in the past multiple times but never had much success. But since I've found a few interesting things, I figured out I'd create an issue to keep them listed.

For a long time I didn't know what library took care of compiling the import/exports. It's a webpack module, but there isn't a lot of documentation on how to customize it. It's the JavascriptModulesPlugin https://github.com/webpack/webpack/blob/master/lib/javascript/JavascriptModulesPlugin.js . There seem to be some interesting options, like for example the ability to disable the IIFE (Immediately invoked function expression, I discovered that word today while reading webpack source code).

Maybe there would be a way to extend the JavascriptModulesPlugin to change from array-based into key-based, so that everything can use the namespaced names, and have everything in the global namespace with a single webpack module loader boot code. Maybe there are some things to look around chunkGraph.getModuleId.

Something which I discovered today and seems very new but also very relevant is https://module-federation.github.io/blog/get-started . It seems to have been recently integrated into Webpack itself https://github.com/webpack/webpack/issues/10352

That library seems to be exactly a hybrid of the built-in import/export plugin AND something like our compat array. It seems like we could use that webpack plugin in our extensions, then specify a module name and a list of file paths to export globally. It's designed to be used from different javascript files loaded from different origins, but it seems like it would work just as well inside a single, dynamically concatenated file.

askvortsov1 commented 3 years ago

I've been thinking about this a bit. Nothing approaching a solution yet, but wanted to add some thoughts:

so it's only possible to monkey-patch a class prototype, but not a simple function. This probably could have a GitHub issue of its own

Agreed it's a separate issue, but frankly I don't think it's possible: class prototypes are mutable, functions are not. The best we could do is collect functions in a central location, export that central location in compat, and mutate that. However, I'm unsure that we'd even want to do this. Instead, perhaps some utils (like the controls stuff) should be classes, and others should just be immutable as a design decision. We could also adopt some kind of use-case-fueled extensibility entrypoints via JS extenders when we start bringing those in. But either way, regardless of what export mechanism we use, I don't think there's a way to change a function in-place.

here's no way to automatically have everything exposed.

I think this is where we need to focus efforts. Maintaining a catalogue of all files is messy, duplicates work, and can easily get out of sync. There's a few considerations here:

Also, a few other thoughts:

All that being said, I dont think that federated modules or a particularly complicated solution is necessary. Frankly, we need to do a thorough revision / cleanup of our JS system, because right now there's a bunch of unused / planned-but-abandoned stuff in there that's confusing to new devs. If we need any of it back, we always have git history.

askvortsov1 commented 3 years ago

I found https://www.npmjs.com/package/webpack-auto-export which aims to accomplish something similar to what we want. Unfortunately, it works by compiling index.jss everywhere, which we could do without a webpack plugin. This got me thinking though:

Now that Flarum CLI is a thing, could we add a script to it that will recursively loop through directories and construct index.js files? To prevent rewriting over literally everything, the code it generates should be wrapped in:

//export
Exports here
//exportend

tags or something similar so that it's clear that it's auto-generated. That'd also make things easy to update: just rewrite the auto-generated section. Throw an error / ask for confirmation if it's missing.

From there, we'd need to decide whether we want the unified exports system to use a variant of compat (if so, can we please rename it to exports or something?), or the index.js method that seems to be a bit more standard (suggested by @clarkwinkelmann in https://github.com/FriendsOfFlarum/byobu/pull/112#issuecomment-632759044 during the discussion for byobu's exports).

clarkwinkelmann commented 3 years ago

Another inconsistency there is with the current system is path vs nested. like do we want extensions['vendor-package'].component.MyComponent or extensions['vendor-package']['component/MyComponent']. The first one is what nested index.js files achieve. However the second one is what we can use with the ext: or @ syntax in imports.

Whatever we chose, I think we need to make it work with proper imports. Those imports should also properly return undefined if a class does not exist instead of throwing an error so we can use them for optional dependencies as well.

dsevillamartin commented 3 years ago

However the second one is what we can use with the ext: or @ syntax in imports.

We can do this with both by replacing flarum.extensions (or whatever it is) with a Proxy like we did with flarum.core.compat. Just a thought.

askvortsov1 commented 3 years ago

by replacing flarum.extensions (or whatever it is) with a Proxy

I like this idea, that should give us robustness for checking existence before necessarily returning. Also gives us a level of flexibility

If we're going auto-generated export files, the decision to make is whether we want a single exports file per-frontend, or whether we want nested index.js. To be honest, a single exports feels like it would be simpler to manage, although I suppose it's somewhat less compliant with convention. Clark, is there a reason you went with nested index.js files in your extensions?

clarkwinkelmann commented 3 years ago

The reason I went with nested components (I don't remember if I started the trend) was the ability for the IDE to check everything for typos. By using nested index.js and spread operators, everything has to have the correct name and everything is checked by the IDE. There was also less risk for conflicting PRs since everything to modify would be in a single folder and not spread across the javascript source code.

Also I don't particularly like having a huge file with tens of imports followed by tens of strings matching those imports. Of course if it's generated automatically it's less of an issue.

Still I'd prefer something completely passive that doesn't require running any script.

Also a word on bundle size: I have extensions with many tens of exported components, I don't like how everything is repeated many times. Bundling a compat-like array with each extension is a waste of bytes if we could somehow register the export automatically when webpack already does it. Having webpack reduce everything to randomly named variables, only to expend it again once it's used or re-exported feels very inefficient.

askvortsov1 commented 3 years ago

https://github.com/flarum/docs/pull/72#issuecomment-526029106 has some rationale on the old API. Probably won't affect our decisions, but still useful to read over it.

askvortsov1 commented 3 years ago

After some investigation, I've compiled my findings in https://discuss.flarum.org/d/26849-how-flarums-frontend-works

Also a word on bundle size: I have extensions with many tens of exported components,

I don't think we can do any better bundle-size-wise than the 3rd option I identified in the "so now what" section of the writeup. But if the only code we're adding is a bunch of imports, and then an export of a giant object, I'm not sure how much better we can do than that. We can't mangle key names, and everything else would already be minified.

askvortsov1 commented 3 years ago

https://github.com/flarum/core/commit/3b2d3dc8bca2edb574fb48cbb5d78fa782261585 contains the foundations of this. Essentially: