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.

Sign up for GitHub

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

Jump to bottom

Emit an error if a relation with a fragment gets redirected #165

Merged
merged 3 commits into from
Oct 29, 2019
Merged

Emit an error if a relation with a fragment gets redirected #165

merged 3 commits into from
Oct 29, 2019

Conversation

papandreou
Copy link
Collaborator

@papandreou papandreou commented Oct 24, 2019 •
edited
Loading

... except through a FileRedirect (implicit += index.html) with a trailing slash on the href.

webpack/webpack.js.org#3332 (comment)

@papandreou
Emit an error if a relation with a fragment gets redirected
a2c4a9e 
... except through a FileRedirect (implicit += /index.html) with a
trailing slash on the href.

webpack/webpack.js.org#3332 (comment)
@papandreou papandreou self-assigned this Oct 24, 2019
@coveralls
Copy link

coveralls commented Oct 24, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 77.067% when pulling 832c009 on feature/warnAboutFragmentRedirects into 007c38e on master.

@papandreou papandreou requested a review from Munter October 24, 2019 22:35
@Munter
Copy link
Owner

Munter commented Oct 25, 2019

Awesome!

I think I need to have a look at changing the error messages for that case. The expected property isn't really helping to surface the problem. And I should probably also add an expected and actual so reports become more explicit

@papandreou
Copy link
Collaborator Author

Yeah, I am not super familiar with how those error reports are supposed to look. Feel free to change them 🤗

Munter
Munter approved these changes Oct 28, 2019
View reviewed changes
Copy link
Owner

@Munter Munter left a comment

Choose a reason for hiding this comment

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

Looks ready to merge to me. Unless you can find any obvious problems with my additions

@papandreou
Copy link
Collaborator Author

LGTM, although I'm a little bit on the fence about assetgraph/lib/util/getHrefType being part of the public api.

@Munter
Copy link
Owner

Munter commented Oct 29, 2019

I think gethreftype should be pulled out into its own module. I can do that

@Munter
Copy link
Owner

Munter commented Oct 29, 2019

https://www.npmjs.com/package/hreftypes now exists, and this PR is using it. I'll make a PR for switching assetgraph over soon

@Munter Munter merged commit b3fb168 into master Oct 29, 2019
@papandreou papandreou deleted the feature/warnAboutFragmentRedirects branch October 29, 2019 18:42
@Munter
Copy link
Owner

Munter commented Oct 29, 2019

Released in v4.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Munter Munter Munter approved these changes

Assignees

@papandreou papandreou

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@papandreou @coveralls @Munter