fix: empty filename should not be treated as string

[edmundhung]

If you create a FormData object on the browser with empty file input, a default empty file entry (i.e. new File([], '')) would be generated. However, this is presented as an empty string instead when you read it on the server.

You can test it here.

I am not sure where to add this test. Let me know if you have any suggestion.

[changeset-bot]

🦋 Changeset detected

Latest commit: e0b02f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@remix-run/web-fetch	Patch

Not sure what this means? Click here to learn what changesets are.

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

[jenseng]

might be worth adding a comment/link here indicating this is exactly what happens to empty file inputs when constructing the form data set (e.g. native form submission, or new FormData(someForm))

[jenseng]

Perhaps a good place to test on the remix side would be in https://github.com/remix-run/remix/blob/main/integration/form-test.ts, i.e. you could a test that's currently expected to fail via test.fail (see other examples in the file), which should then change to passing when this is incorporated into remix

[edmundhung]

Perhaps a good place to test on the remix side would be in https://github.com/remix-run/remix/blob/main/integration/form-test.ts, i.e. you could a test that's currently expected to fail via test.fail (see other examples in the file), which should then change to passing when this is incorporated into remix

If this is considered a patch, it will be applied to existing remix app anytime the package manager updates it. But I am a bit worry as some users might be relying on this behaviour without knowing it was wrong.

export async function action({ request }) {
  const formData = await request.formData();
  const file = formData.get('file');

  // Case 1: Simply check if file is truthy
  if (file) {
    // ...
  }

  // Case 2: Check if it is an file
  if (file instanceof File) {
    // ...
  }
}

Both cases will be always true after the patch. 😅

[jenseng]

Since it's a breaking change from the POV of this package, maybe the changeset should indicate major? That would prevent existing remix versions from picking it up, and you could reliably write a failing test over there.

As far as bumping the web-fetch dep within remix to use this, I don't know if that's big enough to warrant major over there, as it could be argued this is a patchset-level bugfix and is unlikely to actually break anyone 🤷

[brophdawg11]

@jacob-ebey Would love your eyes here. I think this is the equivalent using the node FormData (using an empty buffer) - but it runs into an issue in the npm form-data package that doesn't support empty string filenames - but that's exactly what web browsers submit:

Since it's not an issue for the bug at hand, I figured we could deal with a form-data PR and/or update separately.

[brophdawg11]

I don't think this would warrant a major for this repo? It feels like a bug fix to me since we're not spec-compliant currently. There's always a non-zero change someone is relying on buggy behavior.

[kentcdodds]

Thanks! :)