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

build: relocate site netlify.toml to site/ directory #7090

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

ndhoule
Copy link
Contributor

@ndhoule ndhoule commented Mar 6, 2025

This change moves the site netlify.toml to the site/ directory.

Previously, the site-build workflow went like this:

  • Netlify Build executed npm install at the root of the project (by default)
  • After package installation, the postinstall hook triggered a CLI build
  • Netlify Build would execute netlify.toml#build.command, which:
    • Entered the docs directory
    • Ran npm install against docs/package.json
    • Ran a site build (which implicitly relied on the root directory npm install and npm run build steps)

To make this change, I modified the logic to:

  • Netlify Build starts in the site/ directory and executes npm install
  • site/netlify.toml#build.command enters the root and preps the CLI, then returns to the docs/ directory
  • We then build the site

In this way, the site's dependency on the CLI's source is more clear--we assume nothing about the state of the root directory before building the docs site.

Some other noteworthy changes here:

  • While making these changes, I realized the (JavaScript) site build scripts assume that the CLI has been built to the dist directory. I modified them to be transpiled at runtime and to import from src.
    • I'm not particularly attached to this change, but it reduces the amount of setup steps required and will let us convert these scripts to TypeScript.
  • I moved the site functions to the standard Netlify functions location, site/netlify/functions. Doing so also let me move a dependency that was only used on the site (@bugsnag/js) out of the root, which slims the CLI's dependency tree.
  • I got rid of most of the site: tasks from the top-level package.json. (I left the docs task, though.)
  • These changes require that we change the CLI site's base directory to docs. The site is refusing to build on main right now anyway, so I already made that change.

Copy link

github-actions bot commented Mar 6, 2025

📊 Benchmark results

Comparing with 25fcdd8

  • Dependency count: 1,182 ⬇️ 0.34% decrease vs. 25fcdd8
  • Package size: 281 MB ⬇️ 0.42% decrease vs. 25fcdd8
  • Number of ts-expect-error directives: 724 (no change)

@ndhoule ndhoule force-pushed the build/move-netlify-toml-site branch 10 times, most recently from bd01aec to 5a420dd Compare March 6, 2025 01:23
@ndhoule ndhoule force-pushed the build/move-netlify-toml-site branch from 5a420dd to 3d7e508 Compare March 6, 2025 01:26
@ndhoule ndhoule marked this pull request as ready for review March 6, 2025 03:28
@ndhoule ndhoule requested review from a team as code owners March 6, 2025 03:28
Copy link

@jmarlena jmarlena left a comment

Choose a reason for hiding this comment

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

@ndhoule Assuming the docs CLI site still builds fine with these changes, this is fine to merge! If it's hard to test the site build, we can always merge this and prepare to revert if needed.

@ndhoule
Copy link
Contributor Author

ndhoule commented Mar 6, 2025

@jmarlena Thanks! This is testable (pre-merge) by visiting the deploy preview:

https://deploy-preview-7090--cli.netlify.com/

I did a spot check on a few of the pages by flipping back and forth between the preview and cli.netlify.app and didn't notice any difference.

@ndhoule ndhoule merged commit 1c5db59 into main Mar 6, 2025
49 checks passed
@ndhoule ndhoule deleted the build/move-netlify-toml-site branch March 6, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants