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: use heuristics for build and deploy command as well #6729

Conversation

lukasholzer
Copy link
Collaborator

@lukasholzer lukasholzer commented Jun 24, 2024

🎉 Thanks for submitting a pull request! 🎉

Summary

Depends on: netlify/build#5735

Fixes https://linear.app/netlify/issue/CPLA-738/sites-created-in-the-cli-should-use-framework-detection

Run the framework detection before ntl build and ntl deploy --build as well to not need a netlify.toml in simple cases. (ntl dev already supported this)


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@lukasholzer lukasholzer requested a review from a team as a code owner June 24, 2024 10:39
Copy link

github-actions bot commented Jun 24, 2024

📊 Benchmark results

Comparing with e45d378

  • Dependency count: 1,212 (no change)
  • Package size: 313 MB (no change)
  • Number of ts-expect-error directives: 977 ⬆️ 0.10% increase vs. e45d378

eduardoboucas
eduardoboucas previously approved these changes Jun 26, 2024
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

It'd be nice to have some tests, but LGTM.

@lukasholzer lukasholzer force-pushed the fix/use-heuristics-for-detecting-settings-in-build-and-deploy-as-well branch from 0265b47 to 6b2077b Compare July 11, 2024 09:00
/**
* Merges the settings from the heuristics with the cached config.
* Returns the updated cached config
* @param cachedConfig The cached config
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs updating, since I don't see this param?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh good catch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed here: 049f4a9

eduardoboucas
eduardoboucas previously approved these changes Jul 15, 2024
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

🚀

@lukasholzer
Copy link
Collaborator Author

@eduardoboucas I'm going to add an integration test to it as well :)

@lukasholzer lukasholzer merged commit 111967c into main Jul 15, 2024
48 checks passed
@lukasholzer lukasholzer deleted the fix/use-heuristics-for-detecting-settings-in-build-and-deploy-as-well branch July 15, 2024 17:59
@ascorbic
Copy link
Contributor

This is great! Should it be working for serve as well? I ask because I tried creating a new astro site and then ran ntl serve and it gave me a 404.

@flore2003
Copy link

flore2003 commented Jul 30, 2024

Is there a way to overwrite or turn off this heuristic framework detection for the build and deploy commands? This new behavior breaks our current monorepo setup, because we have both Next.js and Vite based React apps in it, and the heuristic detects next as the framework, even for the Vite based React app. The dev command has an option to specify the framework, but neither build nor deploy seem to have it, if I see it correctly.

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.

4 participants