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[dev-server]: Fix html file matching from URL #9347

Conversation

tanmoyopenroot
Copy link
Contributor

@tanmoyopenroot tanmoyopenroot commented Oct 29, 2023

Closes #9211

↪️ Pull Request

In this PR we rethink how we server the HTML bundle for a given URL in the dev server.

💻 Examples

We get all the htmlBundleFilePaths and sort them based on /(depth)

Example:
For URL /other and let's say we have sorted htmlBundleFilePaths as ['/other.html', 'other/index.html' ]
So for /other URL should be served with /other.html file.

And if we have sorted htmlBundleFilePaths as ['other/index.html' ]
For /other URL should be served with /other/index.html file.

🚨 Test instructions

Extend the html example(packages/examples/HTML).

Create multiple directories like

Screenshot 2023-10-29 at 12 12 16 PM

Add source and dev command in package.json

  "scripts": {
    "dev": "parcel",
    "demo": "parcel build src/index.html"
  },
  "source": [
		"src/index.html",
		"src/other.html",
		"src/temp/index.html",
		"src/temp/other.html",
                 "src/other/index.html",
		"src/other/other.html"
	],

Run yarn run dev to use the local dev server.

Expected Behavior

  • /other?foo=bar correctly returns the content of packages/examples/html/src/other.html
  • /other.html?foo=bar correctly returns the content of packages/examples/html/src/other.html
  • /other correctly returns the content of packages/examples/html/src/other.html
  • /other.html correctly returns the content of packages/examples/html/src/other.html
  • /other/other?foo=bar correctly returns packages/examples/html/src/other/other.html
  • /other/other correctly returns packages/examples/html/src/other/other.html
  • /other/index correctly returns packages/examples/html/src/other/index.html

✔️ 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

@tanmoyopenroot tanmoyopenroot force-pushed the issue-9211/fix-html-file-matching-in-dev-server branch from 384f3c3 to e8d1608 Compare October 31, 2023 00:12
@tanmoyopenroot
Copy link
Contributor Author

Hey @mischnic, Need your review of this PR.

@mischnic
Copy link
Member

mischnic commented Dec 9, 2023

The tests are failing

@tanmoyopenroot tanmoyopenroot force-pushed the issue-9211/fix-html-file-matching-in-dev-server branch from a5b2d56 to 376ed20 Compare December 10, 2023 08:24
@mischnic
Copy link
Member

Please don't change any existing tests, those are there for a reason and need to pass still.
But of course, add new test cases for the linked issue you're trying to fix.

@tanmoyopenroot
Copy link
Contributor Author

tanmoyopenroot commented Dec 11, 2023

Please don't change any existing tests, those are there for a reason and need to pass still.

Screenshot 2023-12-11 at 7 08 44 AM

For the above folder 👆 and If we look at this line, We find for /something and expect contains of rootIndex - even though /something does not exists, which looks like wrong maybe??? For that, I wrote a different test in line

The same goes for the others like this

Just wondering if that is the expected behaviour.

@mischnic
Copy link
Member

No, the existing tests are correct. For a SPA-style app with client-side routing, unknown routes need to return the index page.

To fix #9211, it might be enough to just remove the ?... part before matching it with the existing logic.

@tanmoyopenroot
Copy link
Contributor Author

No, the existing tests are correct. For a SPA-style app with client-side routing, unknown routes need to return the index page.

Oh okay. Then I will make the required changes

@tanmoyopenroot tanmoyopenroot force-pushed the issue-9211/fix-html-file-matching-in-dev-server branch from 376ed20 to f47bfcb Compare December 11, 2023 12:49
@tanmoyopenroot
Copy link
Contributor Author

To fix #9211, it might be enough to just remove the ?... part before matching it with the existing logic.

Added the changes. Thanks for helping me out all the way 😃 .

@tanmoyopenroot
Copy link
Contributor Author

Integration test is failing in windows environment with both node 20 and node 18

Failed to resolve '@parcel/transformer-js/src/esmodule-helpers.js' from 'D:/a/parcel/parcel/packages/transformers/js/src/JSTransformer.js'

Don't think this is related to my changes.

@mischnic
Copy link
Member

Yeah that's fine, you can ignore that. The tests are flakey.

@@ -196,6 +196,12 @@ export default class Server {
});

let indexFilePath = null;
let {pathname: reqURL} = url.parse(req.originalUrl || req.url);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like previously it only looked at req.url, is there a reason why you added originalUrl?

I remember that we had problems with req.originalUrl in the past if you use the proxy in front of the dev server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to make sure we are using the same pathname everywhere like here.

@mischnic mischnic merged commit e4f0a69 into parcel-bundler:v2 Dec 12, 2023
14 of 16 checks passed
@tanmoyopenroot tanmoyopenroot deleted the issue-9211/fix-html-file-matching-in-dev-server branch December 13, 2023 01:11
irismoini pushed a commit that referenced this pull request Dec 13, 2023
Co-authored-by: Tanmoy Bhowmik <tbhowmik2@atlassian.com>
irismoini added a commit that referenced this pull request Dec 13, 2023
* Fix parcel-query

* Modify parcel-query to not require all graphs on start

* Make flow happy

* Initial fix for inspectCache

* Modify fix for inspectCache

* Remove unused requestTypes

* FIX[dev-server]: Fix html file matching from URL (#9347)

Co-authored-by: Tanmoy Bhowmik <tbhowmik2@atlassian.com>

* Modify inspectCache to give info when not all graphs present

---------

Co-authored-by: Tanmoy Bhowmik <tanmoy.openroot@gmail.com>
Co-authored-by: Tanmoy Bhowmik <tbhowmik2@atlassian.com>
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.

Dev server serves wrong page when using query parameters without extension
2 participants