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: honor forced framework for Remix and Hydrogen #5837

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Sep 18, 2024

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes https://linear.app/netlify/issue/FRB-1324/fix-failing-test-related-to-remix-framework-detection-in-cli

Because of the change in #5820 we now have scenarios where forcing framework via netlify.toml#dev.framework that is not honored by framework's detection, would print quite confusing error:

Invalid framework "remix". It should be one of: analog, angular, assemble, astro, blitz, brunch, cecil, create-react-app, docpad, docusaurus, eleventy, ember, expo, gatsby, gridsome, grunt, gulp, harp, hexo, hugo, hydrogen, jekyll, metalsmith, middleman, next, nuxt, observable, parcel, phenomic, quasar, qwik, react-static, redwoodjs, remix, roots, sapper, solid-js, solid-start, stencil, svelte, svelte-kit, vite, vue, vuepress, wintersmith, wmr, zola

It seems like we do have to ensure that if framework is forced - each framework detection need to honor it. Before Remix and Hydrogen adjustments that was actually the case (but we were not verifying it), because custom detect() method in frameworks was not really changing status of something being detected or not - it was just used to enrich default settings (often based on framework version etc).

This adds a generic test testing if forced framework result in it being "detected" (or rather that forcing a framework is being honored) and add fallback cases to Remix and Hydrogen to use previous defaults if framework is forced and we fail to find any of possible configuration files (for classic compiler or vite variant)

It also implements this idea #5820 (comment) instead of current one, so we avoid doing multiple detection passes and use detection information about config file to decide if Vite or Classic Compiler option should be used)


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)

Comment on lines +18 to +19
// this indicate that framework's custom "detect" method doesn't honor the forced framework
throw new Error(`Forced framework "${frameworkId}" was not detected.`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just change error being thrown - previously it would throw

Invalid framework "remix". It should be one of: analog, angular, assemble, astro, blitz, brunch, cecil, create-react-app, docpad, docusaurus, eleventy, ember, expo, gatsby, gridsome, grunt, gulp, harp, hexo, hugo, hydrogen, jekyll, metalsmith, middleman, next, nuxt, observable, parcel, phenomic, quasar, qwik, react-static, redwoodjs, remix, roots, sapper, solid-js, solid-start, stencil, svelte, svelte-kit, vite, vue, vuepress, wintersmith, wmr, zola

which make no sense, so this just adds specific error message for what is actually happening

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be displayed to users? If so, it might be nice to include what action they should be taking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should never be visible to user because I did add test in get-framework.test.ts that checks if we ever hit that scenario - I'm testing it for all the frameworks we have, force the framework and the project looks like empty directory - this won't fully guarantee that it will never happen as only empty directory case is tested, but it should prevent most likely cases from being unnoticed and thus block the merge of changes that would lead to hitting this error case.

The difficulty with providing something actionable to user is that only custom code in each framework .detect() method can cause problems

@pieh pieh marked this pull request as ready for review September 18, 2024 11:33
@pieh pieh requested review from a team as code owners September 18, 2024 11:33
@pieh pieh changed the title test: add check to see if all the frameworks honors forced framework fix: honor forced framework for Remix and Hydrogen Sep 18, 2024
@pieh pieh force-pushed the michalpiechowiak/frb-1324-fix-failing-test-related-to-remix-framework-detection-in-cli branch from 7e19e12 to 1706dca Compare September 19, 2024 10:13
@pieh pieh force-pushed the michalpiechowiak/frb-1324-fix-failing-test-related-to-remix-framework-detection-in-cli branch 2 times, most recently from 20d1c85 to 8da6cc2 Compare September 19, 2024 10:18
@pieh pieh force-pushed the michalpiechowiak/frb-1324-fix-failing-test-related-to-remix-framework-detection-in-cli branch from 8da6cc2 to 2a59c12 Compare September 19, 2024 10:27
@pieh pieh merged commit 938adbd into main Sep 20, 2024
38 checks passed
@pieh pieh deleted the michalpiechowiak/frb-1324-fix-failing-test-related-to-remix-framework-detection-in-cli branch September 20, 2024 09:35
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.

3 participants