Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gatsby): more robust adapter zero-conf handling #38778

Merged

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Dec 22, 2023

Description

Current adapter handling is subpar and it's easy for build to skip using adapters which can result in broken deploys. For example if manually installed adapter by user is not matching the version expected to be used for current version of gatsby - gatsby would silently skip using any adapter and then redirect/rewrites, API functions, DSG/SSR would just not work.

This changes logic a bit so that:

  1. We check if user has installed adapter -> if they did we check if it's compatible with current version of gatsby -> if it is we use it and finish adapter finding logic.
  2. If we didn't finish -> check if we have previously autoinstalled adapter and if it's compatible with current version of gatsby -> if it is we use it and finish adapter finding logic.
  3. If we didn't finish -> we autoinstall adapter compatible with current version of gatsby and use it.

Which means that situation described above wouldn't happen because we would discard incompatible adapter installed manually by user and instead autoinstall required version ourselves.

There were few other "improvements" made here:

  • currently Gatsby fetches adapters manifest from npm/unpkg - which means if there is problem with manifest and we need to fix it, we have to merge/push change and actually publish new version of gatsby. I added preference to pull adapters manifest directly from Github, so manifest changes can be delivered as soon as code is merged/pushed and we don't even need to publish it to npm just to update manifest
  • there were some quirks sometimes with adapters e2e due to how gatsby-dev-cli sets dev versions and semver matching, so added ability to provide custom adapter manifest via env var (and skip pulling manifest from github/npm/unpkg) and used that in our adapters e2e to always allow using dev version of adapter (aka code of adapter in tested branch / backport PR) - fix(gatsby): allow gatsby-adapter-netlify@">=1.0.0 <=1.0.3" for gatsby@<5.12.10 (#38758) #38762 is example of failure with some notes on why exactly it happened

Documentation

Tests

Related Issues

#38752

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 22, 2023
@pieh pieh added topic: adapters Related to Gatsby Adapters and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 22, 2023
Comment on lines +55 to +57
if (!dataToUse && tryGithubBeforeUnpkg) {
dataToUse = await _fetchFile(GITHUB_ROOT, fileName)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optionally this uses github as source of content, to make it faster to deliver adapter version manifest changes to users - using unpkg require npm publish which currently is not the fastest process

@pieh pieh marked this pull request as ready for review January 4, 2024 10:36
import { satisfies } from "semver"
import type { AdapterInit } from "./types"
import { preferDefault } from "../../bootstrap/prefer-default"
import { getLatestAdapters } from "../get-latest-gatsby-files"

const getAdaptersCacheDir = (): string => join(process.cwd(), `.cache/adapters`)
export const getAdaptersCacheDir = (): string =>
join(process.cwd(), `.cache/adapters`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit (not-blocking), could put the path in a constant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably could, tho I vaguely remember a PR long time ago that moved similar path constructor from constant to function for some usecase where CWD changed after modules were imported.

It also was already a function and only change I did was to export it (to use in tests), so IMO there are is not enough benefits to mess with that (cost of running path.join couple of times vs just once is something you can't even measure as variance will be much greater than perf difference), so I will just keep it as-is

@pieh pieh merged commit 7f08e7b into master Jan 9, 2024
34 checks passed
@pieh pieh deleted the fix/adapters-manifest-fail-build-on-incompatible-adapter branch January 9, 2024 09:56
pieh added a commit that referenced this pull request Jan 9, 2024
* feat: prefer github for adapters version manifest

* fix(gatsby): more robust adapter autoinstallation handling to prevent broken deploys

* test: always allow using installed adapter version in e2e-tests/adapter

(cherry picked from commit 7f08e7b)
pieh added a commit that referenced this pull request Jan 9, 2024
* feat: prefer github for adapters version manifest

* fix(gatsby): more robust adapter autoinstallation handling to prevent broken deploys

* test: always allow using installed adapter version in e2e-tests/adapter

(cherry picked from commit 7f08e7b)

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
This was referenced May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: adapters Related to Gatsby Adapters
Projects
Status: Published
Development

Successfully merging this pull request may close these issues.

2 participants