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

Generate correct 404 on export #6274

Closed
wants to merge 3 commits into from

Conversation

Ephem
Copy link
Contributor

@Ephem Ephem commented Feb 12, 2019

This PR fixes a couple of issues around 404-pages when exporting:

  • Default 404.html when exporting is now a correct 404-page (Fixes Error page displays invalid information #6243)
  • If /404.html is not provided in exportPathMap, add the default one
  • Fix one false negative test in /test/integration/export
  • Remove one false negative test in /test/integration/export

The tests matched for /404/ in the html, but the default error page from the static Express server used in the tests includes this, causing false positives.

The test that was removed was should export 404.html instead of 404/index.html. Because this logic only applies to defaultPathMap and this test-suite defines a custom configuration, there is to my knowledge currently no way to test this without adding a new test-suite.

The options as I see it:

  • Be content with removing the test for now
  • Add a new test-suite for export-default (Scope-creep for this PR? Won't have time to do it next few days)
  • Extend logic to apply to custom exportPathMap as well

@@ -28,7 +28,8 @@ process.on(
await sema.acquire()
const { page, query = {} } = exportPathMap[path]
const req = { url: path }
const res = {}
const res = path === '/404.html' ? { statusCode: 404 } : {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proposed solution in #6243 didn't work since err wasn't actually null when exporting. I was a bit worried defaulting to 404 for any scenario directly in _error.js would have the potential to bite us further down the line, so maybe this might work as an alternative?

expect(html).toMatch(/404/)
})

it('should export 404.html instead of 404/index.html', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if creating a new test-suite for this test alone would be worth it considering the added time for CI etc. I think a export-default suite with more tests than this one would make sense though.

@Ephem
Copy link
Contributor Author

Ephem commented Feb 13, 2019

I don't think this PR makes sense now that #6276 closed the issue. We should still add 404.html if not specified in custom config and fix those tests, but it makes more sense to open a new PR for that including a new export-default suite that is needed to fix one of the tests. Closing this meanwhile, I'll hopefully open a new PR for the above towards the weekend.

@Ephem Ephem closed this Feb 13, 2019
@Timer
Copy link
Member

Timer commented Feb 13, 2019

Hey @Ephem sorry about the double work! We'd love to get those tests fixed and look forward to your PR!

@Ephem
Copy link
Contributor Author

Ephem commented Feb 13, 2019

No worries, we approached this mainly from two different issues, it happens. :) I've learned a lot of valuable things and found some other stuff to improve working on this so time wasn't wasted at all! :)

@timneutkens
Copy link
Member

Superseded by #6912

@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 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.

Error page displays invalid information
3 participants