WIP: Transforms API Part 2 - Pre/post transforms

[hutchgrant]

Related Issue

#422

Summary of Changes

• changed the Transform interface to accept request and compilation objects
• changed our serve functionality to extract pre/post plugins from greenwood.config.js with the same format as previous plugins we allowed through webpack etc
• changed our serve functionality to reduce through the transforms, giving our resulting transformed data to the next transform, if shouldTransform() is true.
• created 2 example plugins which demonstrate, pre, default, and post, html transforms all working together simultaneously.
• created a third example plugin which demonstrates a preprocessor transform in a custom sass plugin
• created a fourth example plugin which demonstrates compiling typescript to javascript

Example Plugins

HTML pre/post transforms plugins

• to test run yarn develop and then visit transform-example.html disabled by default. see package for tests.
• you'll see a page that has run through the custom pre-transform plugin (text reads: "test pre process plugin")
• you'll notice its within the app template, so it did run through the HTML default transform.
• you'll see the text "and post process plugin" which means it ran through the custom post-transform as well

Another PR, another day

Sass preprocessor transform plugin

• test by running yarn develop and visiting any page.
• modified the app template to utilize a header component with .scss instead of css to demonstrate that the preprocessor transform functions as expected.
• simply import .scss file into any component with unsafeCSS(mysassfile.scss)

Typescript plugin

• to test, import a .ts file, in current branch one is already imported to the test-sass/header component. It outputs hello world to client console

[hutchgrant]

Currently have an issue with building for prod with sass plugin. Rollup is giving this error for an import of a sass library within a scss file. We're doing @import './components/test-sass/header/vars.scss'; which is just a small file of variables we're importing into a separate scss file. This should be doable(works fine in develop) but rollup throws this (rejected in bundle.js):

done serializing all pages
Error: File to import not found or unreadable: ./components/test-sass/header/vars.scss.
    at options.error (/home/runner/work/greenwood/greenwood/node_modules/node-sass/lib/index.js:290:33) {
  status: 1,
  file: '/home/runner/work/greenwood/greenwood/www/components/test-sass/header/header.scss',
  line: 1,
  column: 1,
  formatted: 'Error: File to import not found or unreadable: ./components/test-sass/header/vars.scss.\n' +
    '        on line 1 of www/components/test-sass/header/header.scss\n' +
    ">> @import './components/test-sass/header/vars.scss';\n" +
    '\n' +
    '   ^\n',
  code: 'PLUGIN_ERROR',
  plugin: 'postcss',
  hook: 'transform',
  id: '/home/runner/work/greenwood/greenwood/www/components/test-sass/header/header.scss',

[thescientist13]

Wow! Lot of good work going on here! 🤩

Haven't had time to dig into the code too much itself but did a cursory glance to provide some initial thoughts and feedback to keep the conversation going.

---

Regarding the build error, what's happening is that right now Rollup only "knows" about JS (similar to webpack) and has to be "taught" everything else via plugins. Just like I've done for HTML and CSS, per the custom Rollup plugins I created. So having it try and process a .ts or .scss out of the box will yield what you see here.

So there are a couple approaches that come to mind that I was thinking about for how we would need to solve this:

1. Do all transforms entirely via the CLI, so the output (including paths in HTML / CSS / JS) come out as web standard compatible (e.g. HTML / CSS / JS), before being fed into Rollup.
2. Reuse transform logic such that any pre processing plugins like a .ts are turned into Rollup plugins on the fly and re-used during the bundle lifecycle.

Doing everything via our CLI would be my preferred control flow, so the CLI only spits out web standards and thus transformation process happens once. Maybe in dev mode we support .ts extensions from the browser on the fly (so a user would literally see a network request for my-component.ts, and in build mode, re-write the files and paths as part of serialization step?

Not sure what your thoughts are on there?

---

Just some additional thoughts / questions / considerations:

1. Can we rename transforms/ -> plugins/transforms/? I think we might end up having other types of default plugins possibly, and also our transforms themselves be exported as plugins, to keep things consistent.
2. Can we try restoring our existing polyfill and google analytics plugins, I think those should probably be easy to restore now.
3. For SASS and TypeScript plugins, we need README.md files for each of those packages. Should we also add a plugins table to the main project README so discoverability is easier for users? I think it might also make sense to submit those as their own PR since there's a lot of moving parts here and through our test cases we should be able to cover all the desired behavior and functionality. (What I would do is make a copy of this branch locally, then grab the code later and copy / paste into a new PR after this one is merged).
4. Our default transforms should be plugins just in the same way our external plugins are. I don't think we should deviate / side step the same API we share with users.
5. We shouldn't need to create "mock" packages just for testing. The existing Polyfill and GA plugins have their own test cases that should still work after implementing the new transform behavior.
6. We should remove the placholder
7. For documentation, I think it's time we will need to start documenting our work for "new" development happening so it doesn't get further out of date / stale. Today I plan on getting all current docs up to date to reflect what is in the current state of the release/0.10.0 branch

For me, I just need to make sure to follow up on

1. New issue custom content type transforms (e.g. .ts, .scss)
2. make an issue for dev vs. prod toggle for our existing plugins (since you wouldn't really want polyfills and GA loading in development)
3. Make sure TODOs from here are part of Roadmap To 1.0 And Beyond #418

[thescientist13]

why does this need to be async? You have to go through each plugin sequentially anyway so this just feels like a lot of extra code / complexity for no real additional value from what I can tell?

[hutchgrant]

because we have to wait for each plugin to complete and we have no idea how long it could take to transpile and could contain its own async code

[thescientist13]

hmm... so the expectation then will be that every transform needs to follow an async paradigm and return a Promise?

Should that then apply to all TransformInterface methods or just the transform method? I think probably just for the transform method itself could make sense. If so, please make sure to document that for users so they know that is in an async method out of the box.

[thescientist13]

These test transform cases should be within the CLI repo, not their own packages.

[hutchgrant]

polyfills plugin has tests within its package?

[thescientist13]

yes, packages manage their own tests. But these are not actual packages though, we would definitely not be publishing these to npm. From what I gather they are they are here to purely to exercise the general business logic around pre and post transforms and that from a generic stand point those things work. (which is great!)

If so, just create a test case in the CLI package, like we have with our previous iterations of plugins, which we can probably all delete now and instead replace with one for testing these transform plugins.

• https://github.com/ProjectEvergreen/greenwood/tree/master/packages/cli/test/cases/build.plugins-index
• https://github.com/ProjectEvergreen/greenwood/tree/master/packages/cli/test/cases/build.plugins-webpack

[thescientist13]

same here re: location (move to cli/tests)

[thescientist13]

So, thinking we should save non web standard content types (.ts, .scss) for a third PR, and just focus on getting the pre / post sequencing established and supporting standard for this PR, which our Rollup configuration should already be able to handle out of the box.

That should still allow us to restore our existing plugins (Polyfills and Google Analytics) as part of this PR, so that would definitely be super valuable to accomplish and a good milestone for this work. We can dedicate time to discussing how to tackle the other content types in our next meeting.

[hutchgrant]

the .ts and .scss plugins are great examples of the limitations of our pre/post transformation methodology. I agree its a seperate issue and should be tackled seperately but its tightly coupled with a completed transforms API. For simplicity sure, but can't really say we have pre/post transform if all we enable is tranforming .css, .js and .html.

[hutchgrant]

We have problems with the polyfills plugin in prod. We have to make an import map for all other dependent libraries it may load. It's a separate issue though that we're aware of relating to any imported libraries in general. Once you've had a look at it, I think we should disable it for this PR in greenwood.config.js and then revisit later.

[thescientist13]

We have to make an import map for all other dependent libraries it may load. It's a separate issue though that we're aware of relating to any imported libraries in general.

I think we just need to scan the HTML file for <script> tags that don't have src attribute, then see if there are any import or paths to node_modules inside it (acorn could do this). If so, emit that file path as a chunk for Rollup. It should handle all the bundling (like it is doing for lit-element, for example). Just like we do for <script> tags with a src attribute.
https://github.com/ProjectEvergreen/greenwood/blob/release/0.10.0/packages/cli/src/config/rollup.config.js#L53

Then we just "catch" the file on the way out and if we need to rename or inline into the HTML or whatever, we can do that as well.
https://github.com/ProjectEvergreen/greenwood/blob/release/0.10.0/packages/cli/src/config/rollup.config.js#L171

Effectively, it's just this issue manifesting itself in another (similar) context. I think making a PR to fix #434 would instrument the mechanism you need for here.

[thescientist13]

Ok had a little more time to think about this and left a comment re: handling JavaScript / CSS in inline <script> / <style> tags and what kind of expectations we can set from a feature perspective. (short and long term)
#434 (comment)

I think I should be able to make the Polyfills plugin work with a little tweak to our current setup via amending the scope of #434 to support node_modules based paths via <script src="..."> or <link href="...">.

Will take a look a look into all this today.

[thescientist13]

I might be missing something here, but I'm having some issues seeing the Google Analytics plugin working when running yarn build. I think you had mentioned it should be working (but not Polyfills), but while i can log and see that the applyTransform method is indeed getting called, I don't see the actual Google Analytics code in the final index.html files in the /public directory?

I think one way to validate the behavior would be to re-enable the test cases we already had for plugins to validate if the API is working the way we expect (and thus allows us to bump our test coverage back up!). I think if you can get the tests for both GA and Polyfills turned back on (and passing, at least for GA) and then the issues you are seeing become more easily reproducible for someone else, then I can definitely jump in and help out here a bit more. Also updated my comment with latest TODOs status. This means we should be able to delete those test packages.

[thescientist13]

Thanks for pushing up your latest changes. I've had some time to review a bit more in depth and I've been able to address the main issue of resolving the web components loader file, something along these lines

async applyTransform(response) {
  
  return new Promise(async (resolve, reject) => {
    try {
      const isProdBuild = process.env.__GWD_COMMAND__ === 'build'; // eslint-disable-line no-underscore-dangle
      const src = isProdBuild
        ? filename
        : `${nodeModuleRoot}/${filename}`;
      const body = response.body.replace(/<head>/, `<head>
        <!-- Web Components poyfill -->
        <script src="/${src}"></script>
      `);

      if (isProdBuild) {
        fs.copyFileSync(
          path.join(process.cwd(), nodeModuleRoot, filename), 
          path.join(this.context.outputDir, filename)
        );
      }

      resolve({
        body,
        contentType: this.contentType,
        extension: this.extensions
      });
    } catch (e) {
      reject(e);
    }
  });
}

I would even go so far as to say that things like GA and Polyfills wouldn't even happen in development, but that would be up to the plugin author to support that. Or for testing, we / they can provide an explicit option to override. That is something I can make an issue for.

This does cause an error for Rollup though, since it's going to try and trace a direct path to webcomponents-loader.js via the <script> tag in index.html to bundle it. (in this case, it still works for us though, at least)

[Error: ENOENT: no such file or directory, open '/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/www/webcomponents-loader.js'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/www/webcomponents-loader.js'
}

But this file in particular is explicitly not supposed to be bundled by its own design.

This was actually something we didn't have to contend with before but I did have a TODO as part of the refactor to come up with some sort of "teardown" or post-build action. 🙃

This is one case though, but it seems like maybe i just need to play around with this design a little bit more, so let me do that.

---

it doesn't look like fonts are correctly loading in development?

[thescientist13]

should these two plugins only need to check for .html or does it have to also do it for .md too? Could these perhaps reuse the business logic of the our default transforms maybe?

Also, we should avoid doing this kind of logic, re: the replace('.html')

`${workspace}/pages${url.replace('.html', '')}`;

[thescientist13]

should we just passthrough context / compilation? Otherwise it feels like we're just going to end repeating every property and have to keep updating / adding as context and config get updated, which seems like it would get annoying over time.

[thescientist13]

Thanks for all your help on driving the design of this feature @hutchgrant . Although we'll be going with #466 , the spirit of Tranforms lives on, just with a different name. 😃

Good design often takes a couple tries to get right, but I think we can add Transforms / Resource to the list of pretty good design decision we've made for this project. And for the other ones, well, we're also doing a pretty good improving those, even coming up with our own solutions when needed. 🌟