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 #4429: dev server entry detection #4638

Closed
wants to merge 2 commits into from

Conversation

moeriki
Copy link
Contributor

@moeriki moeriki commented May 21, 2020

↪️ Pull Request

Fixes #4429.

💻 Examples

Before the entry detection served the first index.html bundle it found.

This would yield a 404 when running any other html file.

parcel serve test.html

I updated the logic, as suggested, to serve any single HTML entry.

🚨 Test instructions

I didn't see any tests for the dev server. Let me know if or where I can do so.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@height
Copy link

height bot commented May 21, 2020

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

You can also use "Close T-X" to automatically close a task when the pull request is merged.

@moeriki
Copy link
Contributor Author

moeriki commented May 21, 2020

I wasn't sure on how to use the bundleGraph. If there is a better way than to use getBundles() I will gladly update.

);
bundle.filePath.endsWith('.html'),
);
const htmlBundle = htmlBundles.length === 1 ? htmlBundles[0] : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to get a test for this.

}
},
);
bundle.filePath.endsWith('.html'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bundle.filePath.endsWith('.html'),
bundle.type === '.html',

@moeriki
Copy link
Contributor Author

moeriki commented May 21, 2020

I will check all your comments and add a few tests.

But now I'm looking at existing tests I broke.

It seems all html bundles have isEntry: true although the tests in question run with a single entry filepath: bundler('/integration/html/index.html'). Would it be possible there is something else going wrong there?

@moeriki
Copy link
Contributor Author

moeriki commented May 21, 2020

I see.

if there are 2 or more html bundles: 404 (because of the ambiguity, maybe the error page should state this as well)

Per suggest I updated logic from "serve first index.html" to "serve any single HTML bundle".

The problem is there are two tests failing with multiple HTML bundles where it serves a 404 now instead of the first index.html as before.

https://github.com/parcel-bundler/parcel/blob/v2/packages/core/integration-tests/test/server.js#L135
https://github.com/parcel-bundler/parcel/blob/v2/packages/core/integration-tests/test/server.js#L391

Both use the fixtures at integration/html/ which contain both index.html and other.html. I hoped isEntry would filter out the other, but it doesn't. Not sure how to filter this correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404 Page not found when serving arbitrary file
3 participants