fix: various fixes to extension dev/test flow (#5885)

* fix: pass pluginsEnv to integration build hooks build step

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.

* fix: resolve integration dev path relative to buildDir

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.

* fix: return integration build plugin path in development

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.

* refactor: print stack on failure to build integration in dev mode

* style: run prettier

* fix: remove duplicative integration install step

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.

packages/build/src/core/build.ts

@@ -449,6 +449,11 @@ const initAndRunBuild = async function ({
   edgeFunctionsBootstrapURL,
   eventHandlers,
 }) {
+  const pluginsEnv = {
+    ...childEnv,
+    ...getBlobsEnvironmentContext({ api, deployId: deployId, siteId: siteInfo?.id, token }),
+  }
+
   const { pluginsOptions: pluginsOptionsA, timers: timersA } = await getPluginsOptions({
     pluginsOptions,
     netlifyConfig,
@@ -469,13 +474,9 @@ const initAndRunBuild = async function ({
     integrations,
     context,
     systemLog,
+    pluginsEnv,
   })
 
-  const pluginsEnv = {
-    ...childEnv,
-    ...getBlobsEnvironmentContext({ api, deployId: deployId, siteId: siteInfo?.id, token }),
-  }
-
   if (pluginsOptionsA?.length) {
     const buildPlugins = {}
     for (const plugin of pluginsOptionsA) {

packages/build/src/install/missing.js

@@ -34,6 +34,8 @@ export const installIntegrationPlugins = async function ({
   logs,
   context,
   testOpts,
+  pluginsEnv,
+  buildDir,
 }) {
   const integrationsToBuild = integrations.filter(
     (integration) => typeof integration.dev !== 'undefined' && context === 'dev',
@@ -46,7 +48,11 @@ export const installIntegrationPlugins = async function ({
     )
   }
   const packages = (
-    await Promise.all(integrations.map((integration) => getIntegrationPackage({ integration, context, testOpts })))
+    await Promise.all(
+      integrations.map((integration) =>
+        getIntegrationPackage({ integration, context, testOpts, buildDir, pluginsEnv }),
+      ),
+    )
   ).filter(Boolean)
   logInstallIntegrations(
     logs,
@@ -64,25 +70,32 @@ export const installIntegrationPlugins = async function ({
   await addExactDependencies({ packageRoot: autoPluginsDir, isLocal: mode !== 'buildbot', packages })
 }
 
-const getIntegrationPackage = async function ({ integration: { version, dev }, context, testOpts = {} }) {
+const getIntegrationPackage = async function ({
+  integration: { version, dev },
+  context,
+  testOpts = {},
+  buildDir,
+  pluginsEnv,
+}) {
   if (typeof version !== 'undefined') {
     return `${version}/packages/buildhooks.tgz`
   }
 
   if (typeof dev !== 'undefined' && context === 'dev') {
     const { path } = dev
 
-    const integrationDir = testOpts.cwd ? resolve(testOpts.cwd, path) : resolve(path)
+    const integrationDir = testOpts.cwd ? resolve(testOpts.cwd, path) : resolve(buildDir, path)
+
     try {
-      const res = await execa('npm', ['run', 'build'], { cwd: integrationDir })
+      const res = await execa('npm', ['run', 'build'], { cwd: integrationDir, env: pluginsEnv })
 
       // This is horrible and hacky, but `npm run build` will
       // return status code 0 even if it fails
       if (!res.stdout.includes('Build complete!')) {
         throw new Error(res.stdout)
       }
     } catch (e) {
-      throw new Error(`Failed to build integration`)
+      throw new Error(`Failed to build integration. Error:\n\n${e.stack}`)
     }
 
     return undefined

packages/build/src/plugins/options.ts

@@ -32,6 +32,7 @@ const tGetPluginsOptions = async function ({
   integrations,
   context,
   systemLog,
+  pluginsEnv,
 }) {
   const pluginsOptionsA = await resolvePluginsPath({
     pluginsOptions,
@@ -51,6 +52,7 @@ const tGetPluginsOptions = async function ({
     integrations,
     context,
     systemLog,
+    pluginsEnv,
   })
   const pluginsOptionsB = await Promise.all(
     pluginsOptionsA.map((pluginOptions) => loadPluginFiles({ pluginOptions, debug })),

packages/build/src/plugins/resolve.js

@@ -33,6 +33,7 @@ export const resolvePluginsPath = async function ({
   integrations,
   context,
   systemLog,
+  pluginsEnv,
 }) {
   const autoPluginsDir = getAutoPluginsDir(buildDir, packagePath)
   const pluginsOptionsA = await Promise.all(
@@ -77,6 +78,7 @@ export const resolvePluginsPath = async function ({
     buildDir,
     context,
     testOpts,
+    pluginsEnv,
   })
 
   return [...pluginsOptionsE, ...integrationPluginOptions]
@@ -164,9 +166,27 @@ const handleMissingPlugins = async function ({ pluginsOptions, autoPluginsDir, m
   return Promise.all(pluginsOptions.map((pluginOptions) => resolveMissingPluginPath({ pluginOptions, autoPluginsDir })))
 }
 
-const handleIntegrations = async function ({ integrations, autoPluginsDir, mode, logs, buildDir, context, testOpts }) {
+const handleIntegrations = async function ({
+  integrations,
+  autoPluginsDir,
+  mode,
+  logs,
+  buildDir,
+  context,
+  testOpts,
+  pluginsEnv,
+}) {
   const toInstall = integrations.filter((integration) => integration.has_build)
-  await installIntegrationPlugins({ integrations: toInstall, autoPluginsDir, mode, logs, context, testOpts })
+  await installIntegrationPlugins({
+    integrations: toInstall,
+    autoPluginsDir,
+    mode,
+    logs,
+    context,
+    testOpts,
+    buildDir,
+    pluginsEnv,
+  })
 
   if (toInstall.length === 0) {
     return []
@@ -188,7 +208,7 @@ const handleIntegrations = async function ({ integrations, autoPluginsDir, mode,
 const resolveIntegration = async function ({ integration, autoPluginsDir, buildDir, context, testOpts }) {
   if (typeof integration.dev !== 'undefined' && context === 'dev') {
     const { path } = integration.dev
-    const integrationDir = testOpts.cwd ? resolve(testOpts.cwd, path) : resolve(path)
+    const integrationDir = testOpts.cwd ? resolve(testOpts.cwd, path) : resolve(buildDir, path)
     const pluginPath = await resolvePath(`${integrationDir}/.ntli/build`, buildDir)
 
     return { pluginPath, packageName: `${integration.slug}`, isIntegration: true, integration, loadedFrom: 'local' }

packages/build/tests/install/fixtures/local_missing_integration/integration/.ntli/build/index.js

@@ -0,0 +1,3 @@
+export const onPreBuild = function () {
+  console.log("Hello world");
+}

packages/build/tests/install/fixtures/local_missing_integration/integration/.ntli/build/manifest.yml

@@ -0,0 +1,2 @@
+name: abc-integration
+inputs: []

packages/build/tests/install/fixtures/local_missing_integration/integration/.ntli/build/package.json

@@ -0,0 +1,7 @@
+{
+  "main": "index.js",
+  "type": "module",
+  "version": "0.0.0",
+  "name": "abc-integration",
+  "dependencies": {}
+}

packages/build/tests/install/fixtures/local_missing_integration/integration/manifest.yml

@@ -1,2 +1,2 @@
-name: test
+name: abc-integration
 inputs: []

packages/build/tests/install/fixtures/local_missing_integration/integration/package.json

@@ -1,5 +1,5 @@
 {
-  "name": "plugin_deps_plugin",
+  "name": "abc-integration",
   "version": "0.0.1",
   "type": "module",
   "scripts": {

packages/build/tests/install/fixtures/local_missing_integration/netlify.toml

@@ -1,5 +1,5 @@
 [[integrations]]
-name = "abc-integration"
+name = "test"
 
 [integrations.dev]
 path = "./integration"

packages/build/tests/install/snapshots/tests.js.md

@@ -1051,7 +1051,6 @@ Generated by [AVA](https://avajs.dev).
       debug: true␊
       repositoryRoot: packages/build/tests/install/fixtures/local_missing_integration␊
       testOpts:␊
-        cwd: ./tests/install/fixtures/local_missing_integration/␊
         pluginsListUrl: test␊
         silentLingeringProcesses: true␊
     ␊
@@ -1070,10 +1069,18 @@ Generated by [AVA](https://avajs.dev).
       dev␊
     ␊
     > Building integrations␊
-       - abc-integration from ./integration␊
+       - test from ./integration␊
     ␊
     > Loading integrations␊
-       - abc-integration␊
+       - test␊
+    ␊
+    test (onPreBuild event)                                       ␊
+    ────────────────────────────────────────────────────────────────␊
+    ␊
+    Hello world␊
+    ␊
+    (test onPreBuild completed in 1ms)␊
+    Build step duration: test onPreBuild completed in 1ms␊
     ␊
     Netlify Build Complete                                        ␊
     ────────────────────────────────────────────────────────────────␊

packages/build/tests/install/tests.js

@@ -153,15 +153,8 @@ test('Install local plugin dependencies: missing plugin in netlify.toml', async
   t.snapshot(normalizeOutput(output))
 })
 
-test('In integration dev mode, install local plugins and install the integration when forcing build', async (t) => {
-  const output = await new Fixture('./fixtures/local_missing_integration')
-    .withFlags({
-      context: 'dev',
-      testOpts: {
-        cwd: './tests/install/fixtures/local_missing_integration/',
-      },
-    })
-    .runWithBuild()
+test.only('In integration dev mode, install local plugins and install the integration when forcing build', async (t) => {
+  const output = await new Fixture('./fixtures/local_missing_integration').withFlags({ context: 'dev' }).runWithBuild()
 
   t.snapshot(normalizeOutput(output))
 })