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

add support for disabling adapters (#38542) #38549

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

techfg
Copy link

@techfg techfg commented Sep 15, 2023

Description

Add support for disabling adapters to work around the issues described in #38542.

Zero-configuration deployments and adapters are a great step forward for Gatsby and looking forward to seeing them expand. In the meantime, hopeful that you will consider this PR that helps address two primary demographics:

  1. Users on a supported platform (e.g., Netlify or migrating from Gatsby Cloud to Netlify or any future supported platform) that need a workaround to some of the limitations & issues of adapters compared to prior deployment techniques (e.g., `gatsby-plugin-netlify, gatsby-plugin-gatsby-cloud) as described in Gatsby Adapters causes regressions & issues with headers & redirects for NETLIFY builds on >=5.12.0 #38542.
  2. Users who require more control over their build process and do not want side-effects occurring outside of their control

Documentation

Updates to docs exist in the PR, if I missed any places that should be updated and/or if the changes made need adjusting, please just let me know, happy to make any changes.

Note - On zero configuration deployments, I removed the first paragraph since it seemed duplicative with the third paragraph.

Tests

Added unit tests and verified with yarn test, yarn jest packages/gatsby/src/joi-schemas, yarn jest packages/gatsby/src/utils/adapter and yarn jest packages/gatsby/src/redux.

Note - When I run yarn test, some tests, outside of the ones I changed/added, are failing. Not sure if this is a local machine issue (possibly I didn't setup correctly) or if the tests are really failing. The 9 failing tests are:

  1. works correctly if GC happen mid running
  2. works correctly if GC happen mid running
  3. works correctly on fields with resolver if GC happen mid running
  4. works correctly on fields with resolver if GC happen mid running
  5. works correctly on fields with resolver if GC happen mid running
  6. should get a promise when set
  7. should not compile gatsby-config.js
  8. should not compile gatsby-node.js
  9. handles "String, Error, pluginName" signature correctly

Related Issues

Issues: #38542, #38541, #33555
Discussions: #38527, #38528

Additional Information

Approach

There were four ways that I felt providing support for disabling adapters could be achieved:

  1. Via adapter config property
  2. Via new environment variable which could be evaluated either:
    1. In the test method of IAdapterManifestEntry
    2. In initAdapterManager
    3. In getAdapterInit
  3. Via a new feature flag
  4. Provide an official noOpAdapter adapter that can be configured via adapter configuration

I decided to go with the first option as it seemed the most explicit (no hidden environment variables) and most straightforward. If the preference is to take a different route with providing support for this (either via one of the other options or another one entirely), please just let me know and I can make the change.

Other issues/questions

  1. While making this change, I noticed two different other potential issues in the same code-path. I did not make any changes to either of these as my goal was to provide support for disabling adapters and keep the changes as small as possible (only 1 line of code in the runtime changed in this PR), however if you feel these changes are warranted, I can make them and/or create new issues to track them.

    1. If adapterFromGatsbyConfig is truthy (e.g., true, a non-empty string, etc.), it will head down the path of a configured adapter but the value from config may not be an object and it may not include adapt method. As it stands, this will result in no adapter being used, no warning emitted to user, etc. but internally, the runtime will believe there is an adapter and all code from that point assumes adapter is an object. Suggestions here are to validate that adapterFromGatsbyConfig is an object and adapt is present (to match the schema validation) and if not, either fail or go down the noOpAdapterManager route.
    2. The adapt function, according to IAdapter is required, however if it's not present, the adapt phase will just be skipped without any warning/error emitted to user which could lead to unexpected outcomes. Suggest either requiring adapt as the type defines or at least emitting a warning.
  2. When adapters are explicity disabled, there may be value in having the telemetry be a different value for when trackFeatureIsUsed (e.g., adapter:no-op:disabled). Also, could update the current adapter:no-op to be adapter:no-op:not-found) so the different reasons for using the noOpAdapterManager can be identified. If updates to any of these are desired, please just let me know, happy to update.

  3. When updating the joi tests, I noticed that simply checking result.value?.<propertyname> for the expected configuration does not appear to be enough to confirm there was no error as it seems that result.value?.<propertyname> always contains the full input even if it was invalid (seems to contradict the joi docs that indicate value will be the validated and normalized value). Given this, I included an assert to ensure there were no errors. Possibly I am making a mistake but if my understanding is correct, it might be advisable to update the other joi positive test scenarios to assert on error property and ensure its toBeNil().

  4. In the manually installing the adapter docs, it mentions that if you'll be on a supported platform for a while, there are benefits to manual configuration. Another benefit not mentioned is that manual configuration will ensure your builds always use the adapter vs. sometimes. For example, when running gatsby build locally, the gatsby-adapter-netlify adapter will not be used (unless manually configured) because !!process.env.NETLIFY || !!process.env.NETLIFY_LOCAL will evaluate to false (unless it's been added to the environment by the user). Having the build always use the adapter, even when local, ensures consistency with the builds on the target platform. There is something to be said for not having the adapter used on every gatsby build locally but I do think its worth mentioning that to mirror the behavior locally, netlify cli should be used so at least the user knows there will be a difference. Additionally, as more adapters are introduced, the test could differ to when/how adapters are automatically configured. I didn't add anything related to any of this in to the docs as I wasn't sure if it was intentionally left out but if desired, I can add a mention of this based on your guide on what to add.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant