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(build-info): fix detection of Remix and Hydrogen sites #5820

Conversation

serhalp
Copy link
Contributor

@serhalp serhalp commented Aug 28, 2024

We noticed recently that the framework detection wasn't working correctly for all Remix sites. This leads to poor DX but it also affects our reporting that informs framework usage on the platform.

So, this has gotten pretty complex 😓:

  • Remix sites can either use Vite or the Remix Classic Compiler
    • These use different config files, different dev and build config, but almost identical npm dependencies (plus vite in the Remix Vite case)
  • Hydrogen (v2) uses Remix and, as of a few months ago, it supports Remix Vite in addition to the Remix Classic Compiler (it only supported this previously)
  • Netlify's Remix packages were renamed in late 2023, but the Remix detection code only listed the old names
    • Luckily, this still happened to work because it also listed the right config file, but only for Remix Classic Compiler sites
    • However this means that in the Remix Vite case we were failing to detect Remix. Sites were categorized as Vite instead.
  • The remix package itself was renamed to @remix-run/dev, @remix-run/serve, etc. in 2023. We hadn't updated this here either, which exarcerbated the above issues.

I added test fixtures from real sites for every case I could identify.

This has gotten pretty complex:
- Remix sites can either use Vite or the Remix Classic Compiler
  - These use different config files, different dev and build config, but almost identical npm
    dependencies (plus `vite` in the Remix Vite case)
- Hydrogen (v2) uses Remix and, as of a few months ago, it supports Remix Vite in addition to the
  Remix Classic Compiler (it only supported this previously)
- Netlify's Remix packages were renamed in late 2023, but the Remix detection code only listed the
  old names
  - Luckily, this still happened to work because it also listed the right config file, but only for
    Remix Classic Compiler sites
  - However this means that in the Remix Vite case we were failing to detect Remix. Sites were
    categorized as Vite instead.
- The `remix` package itself was renamed to `@remix-run/dev`, `@remix-run/serve`, etc. in 2023. We
  hadn't updated this here either, which exarcerbated the above issues.
@serhalp serhalp force-pushed the serhalp/frp-1287-framework-detection-misidentifies-remix-vite-sites-as-vite branch from 7a4dfbb to ab8c8f3 Compare September 11, 2024 21:49
@serhalp serhalp changed the title fix(build-info): fix detection of Remix sites fix(build-info): fix detection of Remix and Hydrogen sites Sep 11, 2024
@serhalp serhalp marked this pull request as ready for review September 11, 2024 21:51
@serhalp serhalp requested review from a team as code owners September 11, 2024 21:51
Comment on lines +57 to +70
const viteDetection = await this.detectConfigFile(VITE_CONFIG_FILES)
if (viteDetection) {
this.detected = viteDetection
this.dev = VITE_DEV
this.build = VITE_BUILD
return this as DetectedFramework
}
const classicCompilerDetection = await this.detectConfigFile(CLASSIC_COMPILER_CONFIG_FILES)
if (classicCompilerDetection) {
this.detected = classicCompilerDetection
this.dev = CLASSIC_COMPILER_DEV
this.build = CLASSIC_COMPILER_BUILD
return this as DetectedFramework
}
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 just occurred to me that I can simplify this implementation a bit. I can just set

configFiles = [...VITE_CONFIG_FILES, ...CLASSIC_COMPILER_CONFIG_FILES]

in the actual class body

and then simplify here to

Suggested change
const viteDetection = await this.detectConfigFile(VITE_CONFIG_FILES)
if (viteDetection) {
this.detected = viteDetection
this.dev = VITE_DEV
this.build = VITE_BUILD
return this as DetectedFramework
}
const classicCompilerDetection = await this.detectConfigFile(CLASSIC_COMPILER_CONFIG_FILES)
if (classicCompilerDetection) {
this.detected = classicCompilerDetection
this.dev = CLASSIC_COMPILER_DEV
this.build = CLASSIC_COMPILER_BUILD
return this as DetectedFramework
}
if (VITE_CONFIG_FILES.includes(this.detection.config)) {
this.dev = VITE_DEV
this.build = VITE_BUILD
return this as DetectedFramework
}
if (CLASSIC_COMPILER_CONFIG_FILES.includes(this.detection.config)) {
this.dev = CLASSIC_COMPILER_DEV
this.build = CLASSIC_COMPILER_BUILD
return this as DetectedFramework
}

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 turned out not to be possible because when the "NPM dependency" detection gets "merged" with the "config file detection" it just picks the object with the highest accuracy and throws out the other, so we don't have .config on the detection object. I could have refactored to fix this, but... meh.

serhalp and others added 6 commits September 12, 2024 18:32
You can't actually call `test()` within a `test.each` callback. I was only doing this because I
can't use `test.for` to access the test context because we're on an old version of vitest... but
anyway, a manual table test works here.
@serhalp serhalp enabled auto-merge (squash) September 13, 2024 11:14
@serhalp serhalp merged commit d7c8400 into main Sep 13, 2024
38 checks passed
@serhalp serhalp deleted the serhalp/frp-1287-framework-detection-misidentifies-remix-vite-sites-as-vite branch September 13, 2024 11:22
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