Maintenance issue: Exporting modules from silverstripe-admin is rigid

[tractorcow]

Right now if you want to add a new module export from silverstripe-admin it requires a new tag of webpack-config, and publishing it to NPM. This increases the risk of circular dependency chain of merge and release cycles, where webpack-config doesn't export modules necessary to complete builds, or alternatively falsely reports external dependencies which aren't a part of silverstripe-admin yet.

A better solution is to have https://github.com/silverstripe/webpack-config/blob/master/js/externals.js dynamically locate and export exposed modules from silverstripe-admin directly, via a separate include file distributed with admin directly.

[unclecheese]

Good observation. Though I'd argue it's always been a bit rigid. Before this change, it still required updating multiple modules to add a new exposed package. Now it involves a version bump and and multiple upgrades across modules. It's definitely gotten more rigid.

I think as a general principle what you're suggesting is sound, but there are some technical limitations we'd have to navigate to get there:

• Dynamic requires() are really dodgy, and don't help us with expose-loader. (The resulting bundle will just write a function to lazy load them, defeating the purpose of expose).
• All webpack-enabled core modules are agnostic of each other, deliberately. We don't know where silverstripe-admin is going to be installed, and we shouldn't enforce it. Without a module manifest, we'd have to get clever about computing where these modules are installed.

In an ideal world, we would make the source of truth a serialised (easily maintained) list of all the exposed packages that admin would read to build its expose-loader calls, and all the client modules would consume to build their externals.

Possible solution:

• Drop an exports.json file in silverstripe-admin that lists all of the exported globals and their paths. Use this to generate a bundle.js full of require('expose-loader.... calls. In order to ensure the dynamically generated bundle.js file was generated before webpack is run, we'd have to update the yarn script to be something like "build": "./generate-bundle.js && webpack -p --progress..." Using the exports.json for client modules would be trivial, as they would only need to parse the list at the time their webpack.config.js is generated to build their externals declaration dynamically.

Alternatively:

• Leave the bundle.js require() calls as the source of truth that gets updated manually, and have the client modules parse the code to compute their externals, e.g. /require\('expose-loader\?(A-Za-z0-9_-]+).../

My preference would be the first option, as it's completely obscured from the developer, provided he or she is using the yarn scripts, and it just creates a more intuitive place to expose globals. client/src/bundles/bundle.js isn't the first place most people would look but something like exports.json at the same level as webpack.config.js is almost self-descriptive. Plus, it's a bit more robust than relying on static analysis of code.

[tractorcow]

Drop an exports.json file in silverstripe-admin that lists all of the exported globals and their paths. Use this to generate a bundle.

This is the kind of thing I was thinking about for "importing" into the webpack-config module.

[tractorcow]

Leave the bundle.js require() calls as the source of truth that gets updated manually, and have the client modules parse the code to compute their externals, e.g. /require('expose-loader?(A-Za-z0-9_-]+).../

You could go the other way, and make bundle.js pull in all exposed modules from exports.json.

[unclecheese]

The key is that bundle.js has to be static. Those expose calls cannot contain expressions.

[tractorcow]

Oh, I see.

[unclecheese]

This would be the idea open-sausages/silverstripe-admin@82ade8b

[unclecheese]

Would be good to get input from @flamerohr here too,

[flamerohr]

I think generating expose calls by generating a list adds more complexity than necessary and distracts from the core of the issue here, externals need to be declared to webpack so that they do not get compiled into the bundle.
And externals need to be dynamic across modules so that we do not require webpack-config needing a version bump before any additional package added is available.

I admit I'm very much against dynamic require/imports, as well as generating an import bundle, so I would like to brainstorm this a bit further and see if there are less complex solutions.

I would like to focus the discussion more to the external list than expose-loader, if that's alright?

[unclecheese]

Sure. So just to be clear, dynamic imports are off the table, as they defeat expose-loader. The options are, as I see it:

• Leave silverstripe-admin alone and add complexity to the webpack-config module, which would likely take on some form of regexing the require(expose-loader?....) calls.
• Add complexity to silverstripe-admin by generating the bundle, and a minor enhancement to webpack-config (loading exports.json into the config)

I think it's important to remember the end game, here, which is to make the entire frontend layer less opaque and more accessible to thirdparty developers. A simple, easy to understand API for exposing your modules to other bundles is a key feature, IMO. I think we're limiting the scope by assuming that silverstripe-admin is the only module that should be exporting globals. Any module should have that option.

Another thing to consider: Could a lot of these expose-loader calls be replaced by Injector.register? That would go a long way to removing some of the magic and dogfooding our own APIs.

[flamerohr]

Me pulling back the focus was because it's not just silverstripe-admin exposing packages for others to use :)
silverstripe-asset-admin exposes a few things as well, most critically the UploadField, so if we get at least silverstripe-asset-admin as a passing usecase, I think we'd be in a good spot.

I wasn't keen on the idea of an exports.js primarily because you really don't know where these files will be and require the thirdparty to define almost every one.
For example, this package can be in a themes folder, or like travis does composer install inside the module folder itself. (even worse maybe? if later we execute the plan to move silverstripe modules to the vendor folder)

Utilising Injector could be a good idea, it would mean we'd need to at least solve exposing Injector which is simple enough.
And then encourage thirdparty modules to import through Injector instead like const UploadField = Injector.get('UploadField')?

[unclecheese]

So what I was envisioning is that modules could make their export.json paths configurable, for instance, in the package.json:

{
...
"ss-exports": ['path/to/exports.json', 'path/to/other-exports.json'];
...
}

That would offer a bit more flexibility and impose less structure on your module.

I think an actionable task we can take out of this discussion is to review our expose-loader calls and replace with Injector. It may feel like an anti-pattern to encourage module developers to use Injector.get() over import, but the reality is that those packages are not modules in the first place. So my position is that using import for a global is the anti-pattern, not the other way around, and for that reason I think Injector makes a lot of sense here. We're sharing globals, not modules.

[flamerohr]

I agree with that action :) and the position on anti-patterns, thinking about it actively exposing our files as globals seem like the anti-pattern.

I would suggest limiting the import to Injector.get() switch over only apply to our own components and files.
Actual modules like react, redux etc would still use import.
So if we're adding a new (actual) modules to expose, it would still have this kind of patch version bump, but at least it'll be far less frequent and quite justifiable for it to happen.

[unclecheese]

Right, good point of clarification, there. So this really becomes more about exposing vendors and using DI for internal packages.

Either way, we would still need to at some point afford thirdparty devs a way of exposing vendor libraries they're using. It's not critical, though. It just means that if you know another module exposes a library you need, you can make your library that much smaller. But if you don't know that, nothing really happens. You just use the local one.

OK, so there's a story card in here. Will draw something up for @chillu .

[flamerohr]

Sounds good :)
Thirdparty dev exposing their own vendor libraries would be more of a "guideline" than a standard that we'd look to provide and enforce, imo. There are so many ways to skin a cat here.

I would assume that the thirdparty would expose their libraries as per the expose-loader, and if the external list is large enough, they can say "please use require(path.resolve('path-to-module/exports.json')) to get all our exposed libraries"
We could work on that part in another story, perhaps.

[flamerohr]

Another thought could be having Injector as a NPM library where silverstripe-admin has a dependency on. And could potentially be used without silverstripe-admin

(mind dumping while I'm working on something else)

[chillu]

I had already tracked this issue in silverstripe/silverstripe-framework#5601, but this one has more detail in it, so closing the existing one as a duplicate.