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

Serve asynchronously compiled inline assets on error pages #56

Merged
merged 9 commits into from Oct 15, 2019

Conversation

chrisfrank
Copy link
Contributor

@chrisfrank chrisfrank commented Aug 19, 2019

Description of Change

On error pages, serve inline styles and images instead of linking to external files.

Related Issue

Fixes #3.

Motivation and Context

We experimented with a synchronous implementation of this in #29, but it complicates the build process. This implementation inlines assets asynchronously on the first error page request, and on subsequent requests serves the compiled assets from memory.

Checklist

  • Ran npm run lint and updated code style accordingly
  • npm run test passes
  • PR has a description and all contributors/stakeholder are noted/cc'ed
  • tests are updated and/or added to cover new code
  • relevant documentation is changed and/or added

server/routes/errors.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@isaacwhite isaacwhite left a comment

Choose a reason for hiding this comment

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

Hey @chrisfrank, thanks so much for updating this from #29. This looks great! I'm curious to get your thoughts on two high level changes prior to merge:

  • Removing public/errors.css from the diff, since the build task should produce this from the underlying scss.
  • Converting the various functions that return new Promise to use an async/await pattern instead. Additionally, we could use util.promisify() or fs.promises to avoid manually wrapping our file reads in a promise.

If we did those two I think we could simplify this diff significantly and we'd be in awesome shape to merge. What do you think?

public/errors.css Outdated Show resolved Hide resolved
server/routes/errors.js Outdated Show resolved Hide resolved
@chrisfrank
Copy link
Contributor Author

Hey @isaacwhite,

As per your suggestions, I've removed public/errors.css from source control and converted all Promise-based returns to async/await. Turns out removing the compiled stylesheet meant I had to adjust the build process for Travis — which may be why I checked it in to begin with? 🤷‍♂ I can't remember what I was thinking, but I definitely agree that the codebase is cleaner without compiled CSS.

Is there anything else you'd like me to address? I should be able to make time this week.

Copy link
Collaborator

@isaacwhite isaacwhite left a comment

Choose a reason for hiding this comment

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

Hey @chrisfrank, thanks for updating this! Locally it looks great, but there's now an unfortunate interaction with the change @afischer just completed in #51: the search styles for a 404 page break without JS.

It sounds like both of you will have some time to look at finalizing these changes this week. After that, I'd be eager to merge this in and put it in our next release. Thanks again!

server/utils.js Show resolved Hide resolved
@chrisfrank
Copy link
Contributor Author

Almost there, @isaacwhite! I'll coordinate with @afischer about fallback styles for the search page. It looks like that will happen any time the JS fails to load, not just on error pages, so I think we should handle it in a way that isn't isolated to the errors.css file.

@chrisfrank
Copy link
Contributor Author

It looks like @afischer added fallback search styles in #65 👍 💯. So I'll pull those changes in, double-check that everything looks like it should, add a comment re: path.posix.basename, and then we should be good to go. Thanks for steering this PR along smoothly, @isaacwhite.

@chrisfrank
Copy link
Contributor Author

@isaacwhite I think I hit everything on our list today. How's this looking to you?

FWIW, I don't think the CI failure under Node 10 is related to anything in this PR. When I run the test suite locally from master, I see that same playlist test failing ~10% of the time. I suspect just triggering a new build will make everything pass.

@isaacwhite
Copy link
Collaborator

@chrisfrank this looks great, thanks for taking the time to polish! Excited to add this as core functionality.

@isaacwhite isaacwhite self-requested a review October 15, 2019 03:58
@isaacwhite isaacwhite merged commit a05a7b8 into nytimes:master Oct 15, 2019
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.

Styles sometimes broken on 403 error pages
2 participants