-
Notifications
You must be signed in to change notification settings - Fork 380
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
feat: run framework detection and run plugins on netlify build #5029
Conversation
📊 Benchmark resultsComparing with 6593bc7 Package size: 222 MB(no change)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I've added a few questions. Could you change the PR name to follow conventional commits? Also a bit more detail in the PR description shopwing step by step exactly what happens. e.g. user runs ntl build
, if the site exists but has no plugins then x, or if it does have plugins, or if the site doesn't exist then y and so on. I mean it's pretty clear but I know with the dev
command, there were quite a few nuances that needed covering.
@ascorbic I've updated the description to be more clear about the flow, and made the rest of the recommended changes. |
const defaultConfig = { build: {} } | ||
|
||
if (settings.buildCommand) { | ||
buildOptions.cachedConfig.config.build.command = settings.buildCommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the sort of question that is tricky to answer without types in place, but is there a chance that some of these intermediate properties will be undefined
, leaving to a reference error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some chasing here, and found that cachedConfig is created by @netlify/config
- and cachedconfig.config.build is always added. https://github.com/netlify/build/blob/d2c74c02280bd2324cb9b933df4be9ba8c399789/packages/config/src/base.js#L32
I'm not sure what I should do to safeguard against that situation popping up if there's a breaking change in @netlify/config
though. The node version doesn't support optional chaining, and I don't want to do 4 different hasOwnProperty
s :/
But wow, wouldn't this all be so much nicer with Typescript 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small nit, but this is otherwise looking great! Excited to see this ship 🚀
…build. next plugin also fails
665d7c1
to
ee780e5
Compare
* feat: getting the plugin when running build. doesn't run on deploy --build. next plugin also fails * feat: running plugin through both deploy --build and build, plugin fails * feat: adding some buildCommand plumbing * chore: changes * chore: removing unused import and console logs * feat: remove check for site id when running build * chore: cleanup * feat: make dist available at all stages * test: add tests * fix: review cleanup * fix: switch from exit to error * chore: changing variable name for clarity * fix: only change config values when both command and directory are found Co-authored-by: Daniel Tschinder <231804+danez@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
hi @sarahetter
|
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #4948
This allows users to run
netlify build
andnetlify deploy --build
, and have framework detection run, enabling build plugins (and the next runtime) to install and run automatically, without requiring a user to runnetlify link
. Running these automatically simplifies the flow for the user, requiring less config, and duplicating recent similar work fornetlify dev
.New flow/scenarios:
ntl build
on a Next.js app with no extra config/netlify.toml/local plugin installed. They are no longer prompted to runntl link
. Framework detection runs, and passes the build directory, build command, and plugins specified by framework-info to the runBuild function. The newest plugin is installed and run.ntl build
on a Next.js app with a netlify.toml / package.json local plugin installed, with an older version of the plugin specified. They are no longer prompted to runntl link
. Framework detection still runs, but the user's local config overrides the version, and the local older version of the plugin is run.ntl deploy --build
on a Next.js app. This is as above in # 1, however as this is a deploy, they will still be prompted to link / create a new site. Framework detection is run, and the newest plugin is installed and run.ntl build
on a static, vanilla javascript website. Framework detection runs, finds no matching framework, and builds as normal.For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)
