Fix typo in manifest regex

[lpsinger]

The character class [A-f0-9] contains a typo: the manifest versions are upper-case hex strings.

This fixes the following CodeQL warning:
https://codeql.github.com/codeql-query-help/javascript/js-overly-large-range/

[changeset-bot]

⚠️ No Changeset found

Latest commit: bda9c78

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[pcattori]

I think the A-f range is intentional as lowercased hex strings are present in manifest versions:

Plus the integration tests that look for those patterns are now timing out with the proposed changes.

Could change to [A-Fa-f0-9] instead to keep current behavior, or if we can guarantee that uppercase won't be used, then we could drop A-F.

[lpsinger]

I think the A-f range is intentional as lowercased hex strings are present in manifest versions:

How very odd. In my Remix 1.19.0 architect project, the filenames have uppercase manifest versions. What hosting target are you using?

[lpsinger]

@pcattori, did we ever find a reason why in my testing the fingerprints were always uppercase but on yours they were lowercase or mixed case? I just have a lingering worry that there might be another bug there, or at least an inconsistency between different hosting targets.

[pcattori]

Could be an OS-related thing (case-sensitive vs case-insensitive filesystems?), but not sure. If it comes up, we can create a separate issue.

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-a179aa7-20230809 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-b1149bb-20230810 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[pcattori]

@lpsinger I've tracked this down! Looks like plugin-remix.js is grabbing the build hash from public/build/manifest-<HASH>.js filename, whereas the rest of the code is looking at the contents of that file, specifically the assets.version field.

Turns out that esbuild uppercases the build hash when including it as part of a filename. By lowercasing that hash before logging it via logDevReady, Remix HMR are flowing correctly.

But something is messing up the HMR mechanism in Remix for the arc template and the grunge stack. Didn't find it before, since arc seems to be forcing a live reload of the app when files change. Not sure why that's happening since livereload option for arc sandbox is supposed to default to false. Even if I explicitly set livereload false in app.arc, it still happens.

[lpsinger]

@lpsinger I've tracked this down! Looks like plugin-remix.js is grabbing the build hash from public/build/manifest-<HASH>.js filename, whereas the rest of the code is looking at the contents of that file, specifically the assets.version field.

Turns out that esbuild uppercases the build hash when including it as part of a filename. By lowercasing that hash before logging it via logDevReady, Remix HMR are flowing correctly.

Wouldn't it be more consistent to have plugin-remix.js check the file contents?

[pcattori]

Wouldn't it be more consistent to have plugin-remix.js check the file contents?

💯 agreed. I already have the change locally but haven't pushed it up as a PR since I've got a couple other fixes I was investigating.