Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Pull request to resolve issue #1442 #1197

Closed
wants to merge 1 commit into from
Closed

Pull request to resolve issue #1442 #1197

wants to merge 1 commit into from

Conversation

jervtub
Copy link

@jervtub jervtub commented May 15, 2020

#1142
Route with address "/client/" is not working.

The first request to, for instance "http://localhost/client/new" would
pass the filter function, as req.start.startsWith(prefix) would match with "/client/".

I assume all the exported files by sapper have an extension, such as "chunk_slug.js" or "slug.svelte". Therefore, adding a new rule to the filter, to ensure each path of a request contains an extension, should resolve the issue.

I have add a test and in addition made sure it both fails without the changes and succeeds with the changes.

#1142
Route with address "/client/" is not working.

The first request to, for instance "http://localhost/client/new" would
pass the filter function, as [`req.start.startsWith(prefix)`](https://github.com/sveltejs/sapper/blob/facbd96e0bd2539acf32c5ac14cd9478c7c635e5/runtime/src/server/middleware/index.ts#L104) would pass.

I assume all the content stored under the manifest has an extension, such as ".js" and ".svelte". Therefore, adding a new filter to have an extension should be sufficient to resolve this issue.
const filter = pathname
? (req: Req) => req.path === pathname
: (req: Req) => req.path.startsWith(prefix);
/*
* [#1442](https://github.com/sveltejs/sapper/issues/1142)
Copy link
Member

Choose a reason for hiding this comment

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

nit: it looks weird that this line is indented more than the previous line that starts the comment. also, usually I would just use the // syntax for a two line comment in the middle of the code

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your feedback, I have withdrawn the pull request to improve it. :)

Copy link
Member

Choose a reason for hiding this comment

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

you can update existing requests. you just make the change, run git commit, and then git push and it will update the pull request

Copy link
Author

Choose a reason for hiding this comment

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

didnt' work out yet, will continue this evening..

Copy link
Author

Choose a reason for hiding this comment

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

you can update existing requests. you just make the change, run git commit, and then git push and it will update the pull request

I'll keep that in mind for any future pull requests I want to update. For now, I took a large detour on updating the pull request..

@jervtub jervtub closed this May 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants