Skip to content

Commit

Permalink
fix: remove duplicative integration install step
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ndhoule committed Oct 19, 2024
1 parent cf8cbf5 commit 9d90cb7
Show file tree
Hide file tree
Showing 11 changed files with 28 additions and 16 deletions.
2 changes: 1 addition & 1 deletion packages/build/src/install/missing.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ const getIntegrationPackage = async function ({
throw new Error(`Failed to build integration. Error:\n\n${e.stack}`)
}

return resolve(integrationDir, '.ntli/site/static/packages/buildhooks.tgz')
return undefined
}

return undefined
Expand Down
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const onPreBuild = function () {
console.log("Hello world");
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
name: abc-integration
inputs: []
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"main": "index.js",
"type": "module",
"version": "0.0.0",
"name": "abc-integration",
"dependencies": {}
}
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
name: test
name: abc-integration
inputs: []
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"name": "plugin_deps_plugin",
"name": "abc-integration",
"version": "0.0.1",
"type": "module",
"scripts": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[[integrations]]
name = "abc-integration"
name = "test"

[integrations.dev]
path = "./integration"
Expand Down
13 changes: 10 additions & 3 deletions packages/build/tests/install/snapshots/tests.js.md
Original file line number Diff line number Diff line change
Expand Up @@ -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␊
Expand All @@ -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 ␊
────────────────────────────────────────────────────────────────␊
Expand Down
Binary file modified packages/build/tests/install/snapshots/tests.js.snap
Binary file not shown.
11 changes: 2 additions & 9 deletions packages/build/tests/install/tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -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))
})

0 comments on commit 9d90cb7

Please sign in to comment.