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

tractorcow commented 1 year ago

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 commented 1 year ago

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

tractorcow commented 1 year ago

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 commented 1 year ago

Aha, great success

I think we are ready to go now?

tractorcow commented 1 year ago

@JoshuaWalsh can you please review

YoshiWalsh commented 1 year ago

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 commented 1 year ago

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. :)

YoshiWalsh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

tractorcow commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

tractorcow commented 1 year ago

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 commented 1 year ago

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

Nantris commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@YoshiWalsh You mentioned here https://github.com/gatsby-uc/gatsby-plugin-s3/issues/422#issuecomment-1483833234 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 commented 10 months ago

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 commented 6 months ago

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 commented 5 months ago

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 commented 4 days ago

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

gatsby-uc / gatsby-plugin-s3

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