Rfc/issue 185 Transforms API (pt. 1)

[hutchgrant]

Related Issue

#185

specifically this comment

Summary of Changes

• Adds standardized transforms class that requires only the request(a modified koa server object) and compilation object(internal greenwood config/context) to instantiate
• per request, if shouldTransform() condition returns true, applyTransform() is called.
• returns obect to possibly be sanitized and then submitted as a response to koa:

resolve({
   body,
   contentType
   extension,
});

example of a javascript transform:

const path = require('path');
const { promises: fsp } = require('fs');
const TransformInterface = require('./transform.interface');

class TransformJS extends TransformInterface {

  constructor(req) {
    super(req, ['.js'], 'text/javascript');
  }

  shouldTransform() {
    const { url } = this.request;

    return this.extensions.indexOf(path.extname(url)) >= 0 &&
      url.indexOf('/node_modules') < 0 && url.indexOf('.json') < 0;
  }

  async applyTransform() {
    return new Promise(async (resolve, reject) => {
      try {

        const jsPath = path.join(this.workspace, this.request.url);
        const body = await fsp.readFile(jsPath, 'utf-8');
        resolve({
          body,
          contentType: this.contentType,
          extension: this.extensions
        });
      } catch (e) {
        reject(e);
      }
    });
  }
};
module.exports = TransformJS;

By extending the transform interface, you will have access to:

    this.extensions = // ['.foo', '.bar'] 
    this.contentType // text/html, text/javascript etc
    this.workspace // greenwood www
    this.outputDir  // public dir
    this.scratchDir // .greenwood
    this.request = koa ctx.request.header and koa ctx.request.url
    this.config // greenwood.config.js

• By default, transforms will only run if the transform extension(declared in constructor) is found in the url file path. You can override this with the shouldTransform() function.

[hutchgrant]

1. Does TransformInterface do a simple passthrough by default? Maybe we could take advantage of that for the prod server, and use to handle the "standard" types since we just need to handle HTML / CSS / JS, which will help us keep the prod server lean e.g. no chance of any pre-processing) so that none of the development logic could accidentally "leak in".

By default there is a serveTransform() function that is called on every transform which extends that interface and can be overriden from each transform. It by default handles any extension type that's declared in constructor. It will serve that file type from the outputDir directory(/public).

2. I see TranformsInterface takes a direct reference to compilation. It was the reason to not pass in ctx directly that we wanted to avoid mutability within userland and so we should aim for the same here and pass in a copy.

Fixed.

3. Could you elaborate a little more on what this filtering code is for / doing?

   // combine arrays and remove duplicates
   return defaultTransforms.concat(transformPlugins.filter(({ extension }) => 
     defaultTransforms.extension.indexOf(extension) < 0));

We're taking an array of the default transforms, we're combining it with an array of custom transforms from the greenwood.config.js we're filtering the default array based on what extension its building for. This for in the case where a custom plugin overrides a default plugin e.g. .html. It's only going to combine in the array unique transforms. The goal is to not have 2 different transforms applying to the same extension type. This logic is problematic and incorrect if multiple extensions within a transform and not a single extension. It therefore requires more work before your point below could be made to work.

4. For folder structure, maybe we should actually export these as plugins and create a plugins/transforms directory? Then I think transform.tools.js would go in lib as transform.utils.js

[thescientist13]

So I hate to be the bearer of bad news or have to say I told you so, but unfortunately all the server refactoring has to be reverted

I probably should have commented after our last team meeting, but the reason why there were two servers was very intentional and was explicit for a reason. The problem it was trying to solve was making sure the final build could be previewed in as faithful of a realistic (albeit hermetic) environment as possible, akin to using a simple web server like Apache, or npx http-server.

What is happening now is that our production build is getting served by the same server that handles all our development transforms, routing, resolutions, etc which would never be possible running on a bare web server. This is effectively giving the false impression that things ara working, when in reality there are not, like double <slot> rendering not happening or prismjs.css loading.

The point of prodServer was to be (intentionally) minimalistic by design to avoid false positives like can be seen here now.

To see what I mean, do this instead

$ rm -rf public
$ yarn build
$ cd public && npx http-server

and you should see all those "fixes" come back. The same would happen if deployed to Netlify.

[hutchgrant]

Right so a typical web server would be running from /public and not configured to serve files how we want anyway. Okay I'll roll it back then.

[thescientist13]

Cool, this seems like a good first step and helps us identify issues and opportunities we will need to solve as we scale this API out to be more flexible and predictable for ourselves and most importantly our users.

I fully assume this design to continue to refine itself over the course of next steps identified in #185 (comment) and good design doesn't typically come fully formed the first time around, and so it's up to us to challenge ourselves to find that diamond in the rough, even if the work is hard. We have the vision at least, which is the most important part.

Will get this all merged up a little later on today.

[hutchgrant]

I think this is part 2/? transforms API . You already merged the initial refactor(#419) of the transforms from the big serve file.

[thescientist13]

Did something happen with the merge? Are there supposed to be two copies of each transform?

[hutchgrant]

It automatically merged those old transforms; I missed it. I only dealt with conflicts.

[thescientist13]

closing and reopening to re-trigger all builds

oh right: Netlify doesn't like non-master based PRs?

[thescientist13]

Cool, good to go. Left some thoughts on the next iteration of this design as well some next steps to consider.