Rfc/issue 185 resources refactor #466

thescientist13 · 2021-01-19T17:03:35Z

Related Issue

resolves #185 - a new take on an existing design as started in #450

Summary of Changes

Introduced the concept of a Resource plugin, and the ResourceInterface base class (f.k.a Transforms) with various lifecycles.
Built in "standard" resource plugins for Greenwood for like JS, CSS, HTML / md, etc. (note: given the tight coupling and shared logic, merged .md and .html back into one)
Refactored Koa middleware logic a bit (and more options possible using Koa Cascading middleware)
There is a TypeScript like example for a .foo resource you can see on the home page
Restored / refactored Google Analytics and Polyfills plugins
Added tests and documentation

I know there is a lot of ground to cover here in this PR, so will go through a full demo / walk through during our next weekly meeting.

Notes / Thoughts

For GraphQL, we should now have the ability to handle @greenwood/data and return JS for .gql
- will likely just need to expose a server's plugin API now
For TS - .foo should provide a basic POC

TODOs

check to see if this new resolve logic now solves / supports resolve (deeply) nested relative template paths to expected workspace path #435 (for its own PR) - left a comment, will need to give Rollup a little help here
move HTML serialize teardown stuff into optimize hook in standard HTML plugin
Need at least one example of intercept - importMap?
remove example .foo code from the website - you can still the same functionality in the test cases for plugins
fix unfinished comment in docs - Note: you can always
More issue / Trello tracking
- do we need to add more font / icon types?
- refactor devServer to use standard plugins now?
- refactoring out parts of StandardHtmlResource plugin like importMap, liveServer into their own?
- should Greenwood standard plugins be pushed to plugins array?

…intercept

hutchgrant

I had got used to calling these transforms plugins ¯\_(ツ)_/¯

That modification to rollup config is perfect, I was thinking people were going to have to include their own configuration with their plugins or something. The additional intercept and optimize capability is a good idea as well.

My only gripe is with no pre-transform(resource) or default override capability. This could be addressed at a later time.

edit: also I'm not sure if I'm in agreement with the combined markdown and html resource. When I wrote #450 and attempted to decouple them, I recall it being slightly difficult because they both needed to make use of app/page templates.

hutchgrant · 2021-01-22T00:05:28Z

packages/cli/src/config/rollup.config.js

+      }
+
+      // handle custom user file extensions
+      const customResources = compilation.config.plugins.filter((plugin) => {


Nice! Was curious how we were going to implement this in rollup. This is exactly whats needed.

hutchgrant · 2021-01-22T00:06:26Z

packages/cli/src/lib/resource-interface.js

+  }
+
+  // eslint-disable-next-line no-unused-vars
+  async intercept(contents, headers) {


Interesting idea +1

hutchgrant · 2021-01-22T00:08:08Z

packages/cli/src/lib/resource-interface.js

+  // ex: remove es shim <script>, convert .ts -> .js and update path references 
+  // this is only an _index.html_ file, BYOA (Bring Your Own AST)
+  // eslint-disable-next-line no-unused-vars
+  shouldOptimize(contents, url) {


Perfect for post-serialize plugin functionality

hutchgrant · 2021-01-22T00:09:26Z

packages/cli/src/lifecycles/serialize.js

-            htmlModified = htmlModified.replace(/<script src="\/node_modules\/@webcomponents\/webcomponentsjs\/webcomponents-bundle.js"><\/script>/, '');
-            htmlModified = htmlModified.replace(/<script type="importmap-shim">.*?<\/script>/s, '');
-            htmlModified = htmlModified.replace(/<script defer="" src="\/node_modules\/es-module-shims\/dist\/es-module-shims.js"><\/script>/, '');
-            htmlModified = htmlModified.replace(/<script type="module-shim"/g, '<script type="module"');


💯 agree with this refactor.

thescientist13 · 2021-01-22T15:24:17Z

My only gripe is with no pre-transform(resource) or default override capability. This could be addressed at a later time.

Yeah, and I think once there are some clear use cases for that, I think it will make it easier to understand what a good design for that could look like. It could be just as easy as running shouldServe before and after maybe?

  // then handle serving urls
  app.use(async (ctx, next) => {
    // run user plugins

    // run greenwood plugins

    // run user plugins again?
  });

Or perhaps cascading middleware is better for this, but certainly solving in its own PR will allow us to drill down into the design and come up with something good once we have that clear use case.

edit: also I'm not sure if I'm in agreement with the combined markdown and html resource. When I wrote #450 and attempted to decouple them, I recall it being slightly difficult because they both needed to make use of app/page templates.

Maybe I misunderstand your comment, but given my experience is the same as yours (decoupling meant more logic spread out in multiple places, specifically shouldServe type logic and thus more bugs), I also found keeping them as one actually makes our code less susceptible to errors as a result and easier to reason about (or at the very least you only have to be looking in one place). So not sure how that makes a case for splitting them up?

* initial transforms refactor using Resources approach * POC of a FooResource * make resource resolve async and resource support for css js and json * support standard font and image resources * integrate markdown resource handling * WIP resource lifecycle refactor * refactor all plugins * comments and code cleanup * delete old transforms * filename change for consistency * custom file extension integration with rollup * fix existing test cases to restore expected outcomes post refactor * fix .ico resolution by using url * made spec for a foo resource plugin * delete old plugin specs, rename existing specs, added plugin name spec * refactor and restore google analytics plugin * refactor and restore polyfills plugin * plugin documentation refactoring * add examples and rename * refactor serialize teardown into standard html optimize * remove unfinished sentance from docs * refactor import map from standard html plugin to node resolve plugin intercept * fix linting * remove example foo code

thescientist13 added 19 commits January 10, 2021 14:53

initial transforms refactor using Resources approach

2016aa4

POC of a FooResource

55e6294

make resource resolve async and resource support for css js and json

c0b51dc

support standard font and image resources

7a138af

integrate markdown resource handling

428e950

WIP resource lifecycle refactor

308b85c

refactor all plugins

f9723eb

comments and code cleanup

e59da4a

delete old transforms

8ce6b7c

filename change for consistency

c37dd9b

custom file extension integration with rollup

d09bf37

fix existing test cases to restore expected outcomes post refactor

9a941d7

fix .ico resolution by using url

c4228c0

made spec for a foo resource plugin

9488822

delete old plugin specs, rename existing specs, added plugin name spec

c4d7fd6

refactor and restore google analytics plugin

a8831aa

refactor and restore polyfills plugin

2fdef53

plugin documentation refactoring

d0330d7

add examples and rename

c85396d

thescientist13 added the RFC Proposal and changes to workflows, architecture, APIs, etc label Jan 19, 2021

thescientist13 requested review from hutchgrant and aholtzman January 19, 2021 17:05

thescientist13 added CLI Plugins Greenwood Plugins P0 Critical issue that should get addressed ASAP labels Jan 19, 2021

thescientist13 self-assigned this Jan 20, 2021

thescientist13 added 4 commits January 20, 2021 10:26

refactor serialize teardown into standard html optimize

9d6c6f8

remove unfinished sentance from docs

41e016f

refactor import map from standard html plugin to node resolve plugin …

6cd7a27

…intercept

fix linting

ba8ba75

remove example foo code

39ba6f0

thescientist13 mentioned this pull request Jan 21, 2021

resolve (deeply) nested relative template paths to expected workspace path #435

Closed

5 tasks

thescientist13 removed their assignment Jan 21, 2021

thescientist13 mentioned this pull request Jan 21, 2021

determine API / usage for plugins #209

Closed

5 tasks

This was linked to issues Jan 21, 2021

determine API / usage for plugins #209

Closed

[RFC] Plugins: Transforms #185

Closed

hutchgrant approved these changes Jan 22, 2021

View reviewed changes

thescientist13 merged commit 48f77f6 into release/0.10.0 Jan 23, 2021

thescientist13 deleted the rfc/issue-185-resources-refactor branch January 23, 2021 17:02

thescientist13 mentioned this pull request Jan 23, 2021

release/0.10.0 #436

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rfc/issue 185 resources refactor #466

Rfc/issue 185 resources refactor #466

thescientist13 commented Jan 19, 2021 •

edited

Loading

hutchgrant left a comment •

edited

Loading

hutchgrant Jan 22, 2021

hutchgrant Jan 22, 2021

hutchgrant Jan 22, 2021

hutchgrant Jan 22, 2021

thescientist13 commented Jan 22, 2021

Rfc/issue 185 resources refactor #466

Rfc/issue 185 resources refactor #466

Conversation

thescientist13 commented Jan 19, 2021 • edited Loading

Related Issue

Summary of Changes

Notes / Thoughts

TODOs

hutchgrant left a comment • edited Loading

Choose a reason for hiding this comment

hutchgrant Jan 22, 2021

Choose a reason for hiding this comment

hutchgrant Jan 22, 2021

Choose a reason for hiding this comment

hutchgrant Jan 22, 2021

Choose a reason for hiding this comment

hutchgrant Jan 22, 2021

Choose a reason for hiding this comment

thescientist13 commented Jan 22, 2021

thescientist13 commented Jan 19, 2021 •

edited

Loading

hutchgrant left a comment •

edited

Loading