[WIP] Split types into Field & Admin-View packages

[jesstelford]

NOTE: I'm am regularly force-pushing to this branch while it's WIP!

The majority of the changes are tweaking the exports of all the field types, and splitting each type into two packages (one bundled by preconstruct for the AdminUI, and the other imported into the node-based Keystone server).

• @voussoir/admin-view-* packages are for displaying in the AdminUI
• @voussoir/field-* packages are for loading into the Keystone instance
• Standardise the exporting from both field-* & admin-view-* packages.
• Bundle the admin-view-* packages with preconstruct.
• AdminUI: Use dynamic import() to load admin-view-* packages only when
  needed by the currently visited list page.

Results in a 3MB bundle size decrease for initial page load of the Admin UI 👍

[emmatown]

Changing the modules option really worries me. There are lots of assumptions here that won't always be true. As an example, let's say i have a src directory with an index.js which contains my keystone server and the node_modules folder is in the parent directory and let's say i run node index.js in the src directory to start the keystone, this will break as the modules won't exist at src/node_modules/.... Also changing the modules option is something which often results in subtle problems with dependencies which make assumptions(which they should be able to make) that the module resolution strategy is exactly as Node specifies.

[jesstelford]

Yeah, I didn't feel great about this either.

The reason I added it was as an odd situation with the meta package @voussoir/fields and peerDependencies. With this structure of splitting the field-* and admin-view-* packages, the admin-view-* becomes a peerDependency of the field-* package (to break the dependency there if the user doesn't want an AdminUI [or builds their own]).

I'm not sure 100% why, but Webpack isn't able to find the dynamically imported modules (the @voussoir/admin-views-* packages) when using the @voussoir/fields package (even though they're specified as dependencies of that package). I have a suspicion it's because we're running webpackdevserver from within the packages/admin-ui folder, and it's not finding the @voussoir/fields package there, but the process.cwd() is the user's keystone directory, and the package is there.

I think this will go away once I pull out the webpack building step into its own build step instead of running webpack dev server.

But in the meantime, if you can think of a way around it, I'd appreciate the help!

[emmatown]

I don't really see why the extra packages are necessary to enable code splitting, a lot of this feels really confusing to me and I don't really understand the reasoning for some of the bigger decisions. Could you explain the decisions more?

Also, it should pretty easy to use suspense for the view loading stuff(i understand they're not always components, i've done some suspense stuff so i could implement that)

[jesstelford]

I don't really see why the extra packages are necessary to enable code splitting

Easy code splitting is one side-effect, but the main focus is to make it easier for folks who want to modify the admin UI to create a new package in whatever way they want to, and set it as the view: config option for a type, without having to modify the type itself at all.

It also breaks the dependency between all the display stuff and the keystone-specific stuff, so if the user doesn't want an AdminUI (or builds their own), they don't have to use any of the Keystone @voussoir/admin-view-* packages.

Also, it should pretty easy to use suspense for the view loading stuff(i understand they're not always components, i've done some suspense stuff so i could implement that)

Oh, I thought you could only use suspense for components? If it's useful for generic dynamic imports, that would be great.

There was a discussion today about taking this concept one step further and dynamically supporting just the Filter or just the Cell or just the Field when it's needed (eg; the Content field's Filter component will be much much quicker to load when all the user wants to do is filter based on some content rather than pull in the entire Field component which drags along slate, etc). But that is out of scope of the work I'm doing right now. If you want to have go after this PR lands; please do!

[emmatown]

but the main focus is to make it easier for folks who want to modify the admin UI to create a new package in whatever way they want to, and set it as the view: config option for a type, without having to modify the type itself at all.

What's stopping someone from doing the code below right now/why is this approach better?

let myTextTypeWithMyOwnViews = { ...Text, views: myOwnViews }

It also breaks the dependency between all the display stuff and the keystone-specific stuff, so if the user doesn't want an AdminUI (or builds their own), they don't have to use any of the Keystone @voussoir/admin-view-* packages.

That's not completely true though since the field type packages have peer dependencies on the views and imo telling people "just ignore the peer dependency warnings" harms the whole js ecosystem because peer dependency warnings exist for a reason and telling people to ignore them in certain cases makes people ignore them in other cases and then have issues. Also, with the current approach, what's the disadvantage of having the views with the field type implementation?

[jesstelford]

What's stopping someone from doing the code below right now/why is this approach better?

Nothing, it's equivalent to:

let myTextTypeWithMyOwnViews = { ...Text, views: 'my-own-views' }

Or, even:

let myTextTypeWithMyOwnViews = { ...Text, views: './views/my-own-text' }

peer dependencies

Right, so perhaps they should be optionalDependencies? The admin-ui is optional, so it makes sense.

harms the whole js ecosystem

Let's not get too far ahead of ourselves. This is a common pattern. For example, see https://knexjs.org/ - they don't appear to use peerDependencies, but they do require the additional installation of another library for things to work.

[emmatown]

Nothing, it's equivalent to:

So if they're equivalent and you can already do it right now, then what's the point of doing this?

Right, so perhaps they should be optionalDependencies? The admin-ui is optional, so it makes sense.

optionalDependencies are still installed by default, it's just that if they fail to install, the install won't fail so I don't think they're right for this.

Let's not get too far ahead of ourselves. This is a common pattern. For example, see https://knexjs.org/ - they don't appear to use peerDependencies, but they do require the additional installation of another library for things to work.

I'm saying we shouldn't use peerDependencies for optional things, knex doesn't use peerDependencies which is exactly what I'm saying, this does use peerDependencies.

[jesstelford]

then what's the point of doing this?

For the other benefits it brings;

• bundling and distributing the packages separately
• making the API for creating your own view more explicit
• putting a wall between the field-* and admin-view-* packages so we can better bundle for deployment to serverless environments and/or create a static build of the AdminUI for deployment to S3, etc.

[emmatown]

bundling and distributing the packages separately

Why is this a good thing? to me, it just seems like it complicates things

making the API for creating your own view more explicit

What exactly does this mean? I find using the word explicit to describe an API is often very vague and doesn't really describe why it's good thing.

putting a wall between the field-* and admin-view-* packages so we can better bundle for deployment to serverless environments and/or create a static build of the AdminUI for deployment to S3, etc.

I don't think this is necessary, we might have to change require.resolve back to path.join with __dirname but I think it's very possible and relatively easy to do without all these extra packages.

[jesstelford]

¯\_(ツ)_/¯

[jesstelford]

With this change, I am able to successfully bundle a Keystone app with @zeit/ncc, which is the tool in use within now for deploying to lambdas.

Without this change, I am not able to even run the bundler.

[jesstelford]

Closing in favour of #746

There is still useful lazy-loading code here that we can cherry-pick in the future.

[JedWatson]

I missed this convo while it was still live but just wanted to chime in with some background...

The whole "field-as-a-package" thing was a really important design constraint for keystone 5 - one of the first things we got working in the POC (glad to see the implementation being revisited now)

The field type API is eventually going to be a massive thing that, if we gets it right, unleashes customisation and extensions at the community level. If you look at eslint, babel, and almost every other major project they do a really good job of implementing the core API then allowing the community to go wild with their own packages on top of that.

I was worried at the start here about complicating that by force-separating each field type's back-end and front-end implementation into different packages. For both publishers (e.g someone who just wants to create a field type without a whole monorepo infrastructure) and consumers (who need to manage peer deps and sync'd version bumps). Also, peerDeps is a nightmare I wish upon nobody.

Basically if there is literally any technical way we can avoid this, imo we should. And early on we did a heap of work to make that possible, because I believe it's so important.

Side-note: keep in mind that right now we're bundling all the field types into a single package for simplicity of early dev; it is absolutely not meant to stay this way. We can have a core set of the basics (text, number, etc) but most fields should end up in their own packages as we approach release.

On a similar note, re:

Bundle the admin-view-* packages with preconstruct

Also before release, we should make sure we have a solid standard setup for every field package that takes its source and transpiles it into vanilla es modules. At the moment the Admin UI build is kind of cheating and doing it for them. That was a pragmatic short-cut because it's nontrivial work to set up the field package(s) build and make sure it works properly with watch mode in dev, which we currently get for free. But we should fix it.

I should probably open an issue about that 🙂