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: various fixes to extension dev/test flow #5885

Merged
merged 7 commits into from
Oct 21, 2024

Conversation

ndhoule
Copy link
Contributor

@ndhoule ndhoule commented Oct 14, 2024

Summary

This changeset fixes several issues that come up when you use @netlify/build programmatically to test integrations extensions' build hooks. A few of these issues can be worked around by running @netlify/build in a subprocess, but some of these make it difficult or impossible to write tests for extension build hooks at all.

This PR is a collection of a few small, related fixes; I added an explanatory commit message for each small commit, collected below so they stick around after the squash. (Happy to break this into three PRs upon request.)

254f154

For context: @netlify/build automatically builds integrations when context is set to dev. This changeset targets that functionality.

Without this change, buildSite's programmatic interface doesn't pass programatically defined env variables into the task that builds integrations extensions' build hooks.

import buildSite from "@netlify/build";

await buildSite({
  env: {
    // This is passed to the build plugin when it's executed, but is not
    // passed to the task that builds the plugin.
    SOME_ENV_VAR: "1",
  },
});

I don't know if pluginsEnv or childEnv is the more appropriate value to pass here, but pluginsEnv seems more consistent with how build treats integrations as plugins, so I chose to use it.

Two total digressions I thought of while writing this:

  • We should probably be passing extendEnv: false to the execa invocation that builds the integration, but I'm not sure if that would break something at this point, so I'm leaving it as is for now.
  • I sort of hate that @netlify/build builds the integration automatically. It's challenging to use it in tests: every test rebuilds the integration (which is expensive) and you have to make sure that no two tests that auto-build the integration build it concurrently, otherwise you might end up with test flakes when one test writes to the built tarball while another test is installing from it. I don't yet know what the best way to solve this problem is, so I'm punting on solving it for now.

3e3234c

Currently, programmatic executions of buildSite resolve a development integration's path (specified via netlify.toml#integrations[].dev.path) relative to the current working directory.

I find this (undocumented) behavior unintuitive; if I specify a relative path in a netlify.toml I would expect it to be resolved relative to the netlify.toml.

This is technically a breaking change, but in practice I really doubt any users are testing extension build hooks, it's unlikely to break anything for the platform team (maybe a few tests, but we can update the paths in those tests if it does).

I'm tempted to remove testOpts.cwd here because I can't find anywhere that we use it internally and it's unintuitive, too, but I'm leaving it in for now as an easy escape hatch in case this change breaks any tests in the SDK.

3f86cd1 / dbd2472

These two commits mainly fix an extension-specific test fixture that was not realistic and broke when I made some of these changes. 9d90cb7 made the fixture realistic and reverted 8972e84, which I realized wasn't actually necessary. (For more context, see the commit message for dbd2472.)

Copy link
Contributor

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@ndhoule ndhoule changed the title Fix/pass env and cwd to integration build context dev fix: various fixes to extension development/test flow Oct 14, 2024
@ndhoule ndhoule changed the title fix: various fixes to extension development/test flow fix: various fixes to extension dev/test flow Oct 14, 2024
@ndhoule ndhoule force-pushed the fix/pass-env-and-cwd-to-integration-build-context-dev branch from cea2fa2 to cf8cbf5 Compare October 14, 2024 23:42
@ndhoule
Copy link
Contributor Author

ndhoule commented Oct 15, 2024

Still need to fix these tests.

For context: `@netlify/build` automatically builds integrations when
`context` is set to `dev`. This PR targets that functionality.

Without this change, `buildSite`'s programmatic interface doesn't pass
programatically defined `env` variables into the task that builds
~integrations~ extensions' build hooks.

```ts
import buildSite from "@netlify/build";

await buildSite({
  env: {
    // This is passed to the build plugin when it's executed, but is not
    // passed to the task that builds the plugin.
    SOME_ENV_VAR: "1",
  },
});
```

I don't know if `pluginsEnv` or `childEnv` is the more appropriate
value to pass here, but `pluginsEnv` seems more consistent with how
build treats integrations as plugins, so I chose to use it.

Two total digressions I thought of while writing this:

- We should probably be passing `extendEnv: false` to the `execa`
  invocation that builds the integration, but I'm not sure if that would
  break something at this point, so I'm leaving it as is for now.
- I sort of hate that `@netlify/build` builds the integration
  automatically. It's challenging to use it in tests: every test
  rebuilds the integration (which is expensive) and you have to make
  sure that no two tests that auto-build the integration build it
  concurrently, otherwise you might end up with test flakes when one
  test writes to the built tarball while another test is installing from
  it. I don't yet know what the best way to solve this problem is, so
  I'm punting on solving it for now.
Currently, programmatic executions of `buildSite` resolve a development
integration's path (specified via `netlify.toml#integrations[].dev.path`)
relative to the current working directory.

I don't think this makes any sense, honestly. I find this undocumented
behavior unintuitive; if I specify a relative path in a `netlify.toml` I
would expect it to be resolved relative to the `netlify.toml`.

This is _technically_ a breaking change, but in practice I really doubt
any users are testing extension build hooks, it's unlikely to break
anything for the platform team (_maybe_ a few tests, but we can update
the paths in those tests if it does).

I'm tempted to remove `testOpts.cwd` here because I can't find anywhere
that we use it internally and it's unintuitive, too, but I'm leaving it
in for now as an easy escape hatch in case this change breaks any tests
Composable Platform has.
Currently, when running a build in `context=dev` mode for a site that
installs an extension never actually installs the extension's build
plugin, if it has one.

That's because the integration package resolver doesn't return the path
to the built package (tarball), so build never picks it up as a plugin.
This changeset fixes that issue.
In 0cfe6d8 I introduced a duplicative integration install step when
`context=dev`, not realizing that we have a special case for resolving
integrations in dev mode:

https://github.com/netlify/build/blob/main/packages/build/src/plugins/resolve.js#L189-L195

Returning the tarball location meant we were installing the
integration's packed tarball and then also installing from the pre-pack
build directory. This changeset removes that duplication.

It's... weird that we don't just return the location of a packed npm
package (tarball) and instead have a special case that points at the
pre-pack build artifact directory. This sort of special-cased action at
a distance makes it super hard to understand how this works. I'm going
to circle back on de-confusing this sometime this quarter when I make
the extension build artifact path configurable, which will solve a lot
of testing pain points we currently have.

The failing test I also fix in this changeset was never realistic and
didn't exercise some of the code paths it was supposed to put under
test, so I've updated that test fixture to be realistic.
@ndhoule ndhoule force-pushed the fix/pass-env-and-cwd-to-integration-build-context-dev branch from 9d90cb7 to dbd2472 Compare October 19, 2024 00:48
Copy link

sonarcloud bot commented Oct 21, 2024

@ndhoule ndhoule enabled auto-merge (squash) October 21, 2024 16:00
@ndhoule ndhoule merged commit 07e567c into main Oct 21, 2024
38 of 39 checks passed
@ndhoule ndhoule deleted the fix/pass-env-and-cwd-to-integration-build-context-dev branch October 21, 2024 16:11
},
})
.runWithBuild()
test.only('In integration dev mode, install local plugins and install the integration when forcing build', async (t) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

😱 @ndhoule quick fix and maybe find the missing eslint rule to enable? 😬

Copy link
Contributor Author

@ndhoule ndhoule Oct 22, 2024

Choose a reason for hiding this comment

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

Oof, sorry about this! I'll fix it this morning and add an ESLint rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants