Migrate aws-sdk from 2 to 3. Update rewrites, gatsby 5 optimisation

[tractorcow]

I have the basic logic and deployment working (with-redirects); I have a proof of concept deployed with a few issues.

Things I need to keep testing:

• Update and run through testing.md (fixed)
• Resolve issue with client-only pages which work differently in gatsby 5 (they still get deployed to s3) (fixed)
• Test with serverless (fixed)
• Home page redirect gives a warning but still work fine (fixed)

Notes from upgrade:

• I've updated each dependency as far as could be done without breakage. I've been testing with gatsby 5 rather than trying to be as backwards-compatible as possible (but it should work fine with older gatsby versions).
• I'm respecting both HTTP_PROXY and HTTPS_PROXY env vars, but I need to confirm how this should be documented.
• I had a few issue with lerna so I ignored it for the most part. Instead a promoted a few dev dependencies from root to the plugin.
• I've changed the way region detection works, but I'm not sure I'm 100% happy with the new logic.
• I fixed a few things I think were bugs. E.g. maxRetries seems to have been ignored, and fixedRetryDelay seems to have been set via env not config.
• I cleaned up as many linting / style errors as I could

The most significant update I've made is to do with rewrites:

• generateRedirectObjectsForPermanentRedirects is now true by default. Turning this off breaks most rewrites, so I'm only testing for when this is enabled.
• Home page redirects no longer uses fake-index.js, and is now a standard file rewrite. The existing solution had some issues, with some warnings (that ended up actually breaking some deployments).
• Wildcard redirects from subpages to parents are disabled (e.g. /sub/* -> /sub) due to them causing infinite loops.
• Wildcard patterns are not implemented at all (e.g. '[...]/index.ts`), outside of client-only paths. There is a note in readme on avoiding them.

With regards to release, it's my recommendation that you release a 1.0.0 under the current codebase, with legacy support for older gatsby versions, and tag this as 2.0.0 beta. I haven't tested this with gatsby 2,3. etc... and it should still work, but I have only tested this with gatsby 5, so you may want to target this version in order to prevent older sites immediately breaking on dependency updates.

[tractorcow]

Fixed up issues with client-only, serverless, and redirects. Need to get e2e testing running and green and we should be good. :)

[tractorcow]

I've come up with a nicer solution for home page redirects (no longer need a dummy js file) and I've implemented a TODO to flip generateRedirectObjectsForPermanentRedirects to true. I also addressed an issue with auto-created buckets.

Still working through e2e testing, but once this is working, I'll be ready to flip from draft to PR :)

[tractorcow]

Aha, great success

I think we are ready to go now?

[YoshiWalsh]

This looks fantastic, thanks for putting the effort into this. My work and life are both very busy at the moment, but I'll try to make some time to review this. (Hopefully on Sunday at the latest)

[tractorcow]

It's ok. We actually critically depend on this module (as I expect many others do), so it's worth my time to invest in updating it. :)

[sanfoobar]

this version should probably be kept as is or increased to something more recent!

[tractorcow]

The version was shifted to terraform.required_providers to upgrade to the new terraform style.

[tractorcow]

See https://registry.terraform.io/providers/hashicorp/aws/latest/docs#example-usage

[sanfoobar]

ah! I missed that, this looks good then. My mistake.

[YoshiWalsh]

Hi @tractorcow , I tried to test this out today but I ran into a couple of issues.

\1. I'm unable to run npm ci in the root project, npm reports that package-lock.json is out of sync with package.json.

\2. I'm unable to build the project using npx lerna run build, I get the following error:

src/bin.ts(460,9): error TS7006: Parameter 'args' implicitly has an 'any' type.
       src/bin.ts(460,9): error TS2769: No overload matches this call.
         The last overload gave the following error.
           Argument of type '(args: any) => any' is not assignable to parameter of type '{ [key: string]: Options; }'.
             Index signature for type 'string' is missing in type '(args: any) => any'.

\3. I'm unable to build the gatsby-plugin-s3 project with npx tsc --project .. I get the following error:

src/bin.ts:460:9 - error TS7006: Parameter 'args' implicitly has an 'any' type.

460         args =>
            ~~~~

  The last overload gave the following error.
    Argument of type '(args: any) => any' is not assignable to parameter of type '{ [key: string]: Options; }'.
      Index signature for type 'string' is missing in type '(args: any) => any'.

460         args =>
            ~~~~~~~
461             args
    ~~~~~~~~~~~~~~~~
...
473                     // eslint-disable-next-line @typescript-eslint/no-explicit-any
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
474                 }) as any,
    ~~~~~~~~~~~~~~~~~~~~~~~~~

  ../node_modules/@types/yargs/index.d.ts:154:9
    154         command<O extends { [key: string]: Options }>(
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    155             command: string | ReadonlyArray<string>,
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ...
    160             deprecated?: boolean | string,
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    161         ): Argv<T>;
        ~~~~~~~~~~~~~~~~~~~
    The last overload is declared here.


Found 2 errors in the same file, starting at: src/bin.ts:460

I'm not sure why running the build through Lerna vs directly yields different results. I did some additional tests and found that both methods definitely use the workspace version of TypeScript. (Not the globally installed version)

Please let me know if there's something I'm doing wrong. Thanks!

[tractorcow]

hi @JoshuaWalsh I gave you write access to the fork in case you wanted to push up any changes. I'll get back to you today on this.

[tractorcow]

Typescript errors have been fixed. My mistake was building at the module level, not using lerna.

[tractorcow]

I think there's still a minor issue where typescript 4 / 5 might be used in different build contexts. It still builds correctly in either case. :)

[YoshiWalsh]

I'm encountering a strange issue while trying to test this on Windows. The e2e script is failing to build the sample sites:

 building site gatsby-plugin-s3-example-with-redirects.
    success compile gatsby files - 0.787s

     ERROR #10123  API.CONFIG.LOADING

    We encountered an error while trying to load your site's gatsby-config. Please
    fix the error and try again.

      Error: Only URLs with a scheme in: file, data, and node are supported by the d
      efault ESM loader. On Windows, absolute paths must be valid file:// URLs. Rece
      ived protocol 'c:'

It's strange because if I manually run the same node command that buildSite is generating in the same working directory, it works perfectly. I'll take more of a look when I have a bit more time.

I did try modifying the buildSite function to run gatsby -v instead of gatsby build and it's definitely using 5.11.0, which has gatsbyjs/gatsby#37251 fixed.

[tractorcow]

Ah, full disclosure, I have been testing on OSX, but I can try to setup a windows environment to test at some point too. I'll have a look over the diffs to see if anything jumps out as a possible red flag.

[tractorcow]

Are you able to do a build on the gatsby-plugin-s3-example-with-redirects manually via npm run build in that folder? (you may need to use an .env file)

[tractorcow]

Here's something that might work. Try testing with node 18/20

https://stackoverflow.com/questions/69665780/error-err-unsupported-esm-url-scheme-only-file-and-data-urls-are-supported-by

My problem is because my Node.js version is too low. Upgrade to Node.js 16 solved the problem.

[YoshiWalsh]

I suspect the issue I'm having with gatsby-config isn't introduced by one of your code changes, but rather is caused by upgrading the Gatsby version. I am using Node 18, as the new Gatsby version required me to update.

npm run build in that directory works without any issues. I also tried modifying the runScript function to use spawn instead of fork, but got the same result. There must be some difference between the way Node is spawning the process and the way PowerShell does. Perhaps it's related to connecting stdio to pipes or something. I'll keep looking into it whenever I have time.

[tractorcow]

Thanks for giving it a good attempt; There may be some differences since I am on OSX and you are on windows, but at least between the two of us we can get a good OS testing coverage. :)

[dan-lind]

Any chance of moving this forward anytime soon @JoshuaWalsh? This looks like a great addition

[tractorcow]

Is there a way of doing a limited beta release that will let other users test and provide feedback? That might take some pressure off @JoshuaWalsh to do all the testing on his end.

[dan-lind]

Good idea, we would be happy to help test this!

[Nantris]

It seems a lot of effort was put into this PR. I hope a maintainer can help get this moved along.

PS: We don't need the change even - it's just a shame for good effort to go to waste.

[afladmark]

If it helps at all to have a user chime in, I've been running this branch live in production for about 2 months with no issues. Running against a ~3500 page Gatsby 4 deployment.

[dan-lind]

@YoshiWalsh You mentioned here #422 (comment)
that you discussed with jariz and took over as maintainer of the project. What needs to happen for this PR to make it into the master-branch?

[tractorcow]

I'm glad to hear others are finding this work helpful. I've also been using this on a few projects, but I understand that testing can be complicated.

I suggest that we split this into a new major version to minimise the risk of existing sites unexpectedly breaking, especially with the lack of regression testing on older gatsby versions (although happy to hear 4 works as well as 5).

Strictly this is a breaking change, so it would be good to have a good 1.0.0 (current) with 2.0.0 (this pr).

[tractorcow]

I made an attempt to publish this to https://www.npmjs.com/package/@pixelfusion-nz/gatsby-plugin-s3

Forgive me if I made a mistake, this is my first time publishing an npm package. :)

[tractorcow]

I've updated https://www.npmjs.com/package/@pixelfusion-nz/gatsby-plugin-s3 with a readme

npm i @pixelfusion-nz/gatsby-plugin-s3

I'll be testing this over the next week with some of my other projects to make sure I've published the package correctly.

[tractorcow]

I've been using @pixelfusion-nz/gatsby-plugin-s3 for a good few months without problems. I realise now that I forgot to enable issues on the repo, so if anyone has problems using it please raise a ticket.

https://github.com/pixelfusion/gatsby-plugin-s3/issues