Fix dev server hanging on binary files #1017

drwpow · 2020-09-08T18:50:44Z

Discussion: #1016

Changes

We were silently processing binary files, trying to scan them for imports, when we shouldn’t.

This PR makes a change where extensions are no longer used to determine if something is a binary file or not. That was error-prone, and we likely get it wrong. We now use isbinaryfile which should be more reliable.

To be clear, our entire build pipeline & plugin workflow still operates by extensions, as it should remain. But us reading files is completely unrelated, and I think relying on extensions too much there have bitten us before. This change should simplify things and make it more automatic.

Testing

dev server now working with repro: https://github.com/ralphtheninja/snowpack-dev-problem
all create-snowpack-app templates tested
all tests passing

vercel · 2020-09-08T18:50:49Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/haxmfz204
✅ Preview: Canceled

[update for 2cb3fbd canceled]

drwpow · 2020-09-08T20:02:00Z

snowpack/src/scan-imports.ts

        return parseJsForInstallTargets(extractJSFromHTML({contents, baseExt}));
      }
-      default: {


This was the source of the bug: we were trying to parse binary files for imports, but hanging on the error.

drwpow · 2020-09-08T20:02:19Z

snowpack/src/scan-imports.ts

+      case '.jsx':
+      case '.mjs':
+      case '.ts':
+      case '.tsx': {


Now we’re more explicit in when we scan JS files for imports.

Not opposed to this as a larger direction, but in the interest of not unintentionally breaking users could we instead use that "is binary data" check so that we don't introduce a huge change here? Considering that this will most likely go out in a bug fix, I'd like to minimize the change if possible.

since we're doing the filter below as well, could we walk this switch statement change back out and still fix the breaking issue for 2.10.2, and then re-introduce the switch statement change in 2.11.0? I'd hate to release a patch release for this that then unnecessarily breaks someone else.

could we instead use that "is binary data" check so that we don't introduce a huge change here?

We actually can’t, because we load string files like .svg and .txt and are parsing them for imports. Before we were passing more things to parseJsForInstallTargets() than I think we realized, and it was just silently working.

I don’t think reverting is safe here; I think reverting is unworkable because we had errors with files before, just different types. +1 agree that patches should change as little as possible. But I think this patch unblocks the most people.

Also to be clear, the loading of .svg and .txt files probably shouldn’t happen, but I think that trying to change that would be a riskier change.

drwpow · 2020-09-08T20:06:44Z

snowpack/src/scan-imports.ts

      return {
        baseExt,
        expandedExt,
        locOnDisk: filePath,
-        contents: await fs.promises.readFile(filePath, 'utf-8'),


The underlying issue: we were trying to enforce binary files be code, which led to breakages when we tried to parse that nonsense code

drwpow · 2020-09-08T20:13:30Z

test/integration/error-include-ignore-unsupported-files/expected-output.txt

@@ -1,2 +1 @@
-[snowpack] ignoring unsupported file "src/c.abcdefg"


This was the only test change: now we don’t throw a warning; we just silently skip it.

Reason is that we were accidentally scanning a lot more text files than intended .json, .txt, but do we surface to the user that we are skipping those? Instead, this PR moves this message to a debug message visible with --verbose.

drwpow · 2020-09-08T20:17:23Z

snowpack/src/commands/install.ts

@@ -457,17 +458,23 @@ export async function getInstallTargets(
 export async function command(commandOptions: CommandOptions) {
  const {cwd, config} = commandOptions;

+  logger.debug('Starting install');


This PR added a few new debug statements for --verbose mode

drwpow · 2020-09-08T20:19:01Z

snowpack/src/types/snowpack.ts

@@ -15,11 +15,11 @@ export type SnowpackBuiltFile = {code: string | Buffer; map?: string};
 export type SnowpackBuildMap = Record<string, SnowpackBuiltFile>;

 /** Standard file interface */
-export interface SnowpackSourceFile {
+export interface SnowpackSourceFile<Type = string | Buffer> {


We need to start a discussion on whether or not we load binary files in Snowpack (sure, we need to copy them over, but should we read them?) However, simply ignoring binary files isn’t a quick change as it affects all of Snowpack.

I know this PR seems like it goes in the direction of reading buffer contents, but it doesn’t. This PR simply exposes the fact we were already doing this (that’s where the bug was), and we make it more transparent to the type system.

drwpow · 2020-09-08T20:21:53Z

snowpack/src/util.ts

-export function getEncodingType(ext: string): 'utf-8' | undefined {
-  return UTF8_FORMATS.includes(ext) ? 'utf-8' : undefined;
+/** Read file from disk; return a string if it’s a code file */
+export async function readFile(filepath: string): Promise<string | Buffer> {


We replaced getEncodingType with a universal readFile function. The way isbinaryfile works, we actually need to read the file contents to determine if it’s a binary file or not, so it makes sense to combine them into one. The end result is this:

const contents = await readFile(filepath); if (typeof contents === 'string') { // This is a UTF-8 file (.js, .css, etc.) } else { // This is a binary file (.jpg, .png, etc.) }

drwpow · 2020-09-08T20:22:52Z

snowpack/src/commands/dev.ts

@@ -166,7 +166,7 @@ const sendFile = (
  }

  res.writeHead(200, headers);
-  res.write(body, getEncodingType(ext) as BufferEncoding);
+  res.write(body);


This is a benefit of this approach: by not caring about encoding types, we’re less likely to get it wrong.

snowpack/src/commands/build.ts

FredKSchott

awesome investigation and fix yall. Left some comments, but in the interest of getting this out consider none of them blocking.

@drwpwr curious which change introduced this between 2.10.1 and 2.10.2? Or, is this not actually a new issue?

FredKSchott · 2020-09-08T21:18:21Z

snowpack/src/commands/install.ts

  }
  if (finalResult.stats) {
    logger.info(printStats(finalResult.stats));
+    logger.debug('Stats printed');


nit: this one feels more unnecessary than the others since the stats printing seems to signify the same. Just a thought, don't feel too strongly

FredKSchott · 2020-09-08T21:20:34Z

snowpack/src/scan-imports.ts

+      case '.jsx':
+      case '.mjs':
+      case '.ts':
+      case '.tsx': {


Not opposed to this as a larger direction, but in the interest of not unintentionally breaking users could we instead use that "is binary data" check so that we don't introduce a huge change here? Considering that this will most likely go out in a bug fix, I'd like to minimize the change if possible.

FredKSchott · 2020-09-08T21:21:53Z

snowpack/src/scan-imports.ts

@@ -273,7 +264,8 @@ export async function scanImportsFromFiles(
  config: SnowpackConfig,
 ): Promise<InstallTarget[]> {
  return loadedFiles
-    .map(parseFileForInstallTargets)
+    .filter((sourceFile) => !Buffer.isBuffer(sourceFile.contents)) // filter out binary files from import scanning


if we already filter by .js, .ts, etc, above should we also filter here? Or, should this really be a warning of "hold up, why is there binary data in a JS file?"

That’s kind of the thing—this install step runs in both build and dev and the reading of those files happens outside this function sometimes. So in order to not have this, we’d have to make an even more sweeping change.

Example:

if (scannedFiles) { // we scanned files in `install.ts` and pass it to `scanImportsFromFiles()` installTargets.push(...(await scanImportsFromFiles(scannedFiles, config))); } else { // we didn’t scan files, so we call scanImports() which scans its own files then calls `scanImportsFromFiles()` anyway installTargets.push(...(await scanImports(cwd, config))); }

We have to handle many different places within Snowpack reading files, which means multiple code paths where binary files could slip through. I could change all that, but that would have been a bigger PR.

got it, thanks for explaining 👍

FredKSchott · 2020-09-08T21:25:53Z

snowpack/src/scan-imports.ts

+      case '.jsx':
+      case '.mjs':
+      case '.ts':
+      case '.tsx': {


since we're doing the filter below as well, could we walk this switch statement change back out and still fix the breaking issue for 2.10.2, and then re-introduce the switch statement change in 2.11.0? I'd hate to release a patch release for this that then unnecessarily breaks someone else.

drwpow · 2020-09-08T21:37:16Z

curious which change introduced this between 2.10.1 and 2.10.2? Or, is this not actually a new issue?

It was introduced by #882, which fixed some of our encoding errors, but not all (it fixed some issues but introduced others). Hoping this PR fully fixes it 🤞🏻

ralphtheninja · 2020-09-08T21:46:33Z

It was introduced by #882, which fixed some of our encoding errors, but not all (it fixed some issues but introduced others). Hoping this PR fully fixes it 🤞🏻

I believe it was introduced by #986, which was the first bad commit between 2.10.1 and 2.10.2. See https://github.com/pikapkg/snowpack/pull/986/files#r485214929

#1016

This makes the string vs buffer handling less brittle by centralizing the logic

drwpow requested a review from a team as a code owner September 8, 2020 18:50

drwpow force-pushed the bugfix-dev-server branch from 78b9f3c to ba0518c Compare September 8, 2020 18:51

vercel bot temporarily deployed to Preview September 8, 2020 18:51 Inactive

vercel bot temporarily deployed to Preview September 8, 2020 20:01 Inactive

drwpow commented Sep 8, 2020

View reviewed changes

drwpow force-pushed the bugfix-dev-server branch from 71cbb6c to 49c2572 Compare September 8, 2020 20:07

vercel bot temporarily deployed to Preview September 8, 2020 20:07 Inactive

drwpow commented Sep 8, 2020

View reviewed changes

drwpow force-pushed the bugfix-dev-server branch from 49c2572 to 641b4ab Compare September 8, 2020 20:16

vercel bot temporarily deployed to Preview September 8, 2020 20:16 Inactive

drwpow commented Sep 8, 2020

View reviewed changes

drwpow requested a review from gr2m September 8, 2020 20:32

ralphtheninja reviewed Sep 8, 2020

View reviewed changes

snowpack/src/commands/build.ts Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview September 8, 2020 21:04 Inactive

FredKSchott approved these changes Sep 8, 2020

View reviewed changes

vercel bot temporarily deployed to Preview September 8, 2020 21:46 Inactive

drwpow added 4 commits September 8, 2020 15:51

Fix dev server hanging on binary files

e259d77

#1016

Add readFile util

43ff1de

This makes the string vs buffer handling less brittle by centralizing the logic

Fix typo

7e30b8c

Remove unnecessary debug

f86f7b6

drwpow force-pushed the bugfix-dev-server branch from 7348bfe to f86f7b6 Compare September 8, 2020 21:52

vercel bot temporarily deployed to Preview September 8, 2020 21:52 Inactive

Fix rebase accident

2cb3fbd

vercel bot temporarily deployed to Preview September 8, 2020 21:54 Inactive

drwpow merged commit 236ba63 into master Sep 8, 2020

drwpow deleted the bugfix-dev-server branch September 8, 2020 22:13

drwpow added a commit that referenced this pull request Sep 8, 2020

Fix dev server hanging on binary files (#1017)

286e10f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dev server hanging on binary files #1017

Fix dev server hanging on binary files #1017

drwpow commented Sep 8, 2020 •

edited

Loading

vercel bot commented Sep 8, 2020 •

edited

Loading

drwpow Sep 8, 2020

drwpow Sep 8, 2020

FredKSchott Sep 8, 2020

FredKSchott Sep 8, 2020

drwpow Sep 8, 2020 •

edited

Loading

drwpow Sep 8, 2020

drwpow Sep 8, 2020

drwpow Sep 8, 2020

drwpow Sep 8, 2020 •

edited

Loading

drwpow Sep 8, 2020 •

edited

Loading

drwpow Sep 8, 2020

drwpow Sep 8, 2020

FredKSchott left a comment

FredKSchott Sep 8, 2020

FredKSchott Sep 8, 2020

FredKSchott Sep 8, 2020

drwpow Sep 8, 2020 •

edited

Loading

FredKSchott Sep 8, 2020

FredKSchott Sep 8, 2020

drwpow commented Sep 8, 2020

ralphtheninja commented Sep 8, 2020 •

edited

Loading

		@@ -1,2 +1 @@
		[snowpack] ignoring unsupported file "src/c.abcdefg"

Fix dev server hanging on binary files #1017

Fix dev server hanging on binary files #1017

Conversation

drwpow commented Sep 8, 2020 • edited Loading

Changes

Testing

vercel bot commented Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

drwpow Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow commented Sep 8, 2020

ralphtheninja commented Sep 8, 2020 • edited Loading

drwpow commented Sep 8, 2020 •

edited

Loading

vercel bot commented Sep 8, 2020 •

edited

Loading

drwpow Sep 8, 2020 •

edited

Loading

drwpow Sep 8, 2020 •

edited

Loading

drwpow Sep 8, 2020 •

edited

Loading

drwpow Sep 8, 2020 •

edited

Loading

ralphtheninja commented Sep 8, 2020 •

edited

Loading