Skip to content
This repository was archived by the owner on Jun 10, 2019. It is now read-only.

Various performance fixes #932

Merged
merged 8 commits into from
Apr 21, 2018
Merged

Various performance fixes #932

merged 8 commits into from
Apr 21, 2018

Conversation

kylemh
Copy link
Member

@kylemh kylemh commented Mar 25, 2018

Description of changes

  1. Use defer or async on Raven.
  2. Delete everything referencing react-table
  3. Bundle notification.css via import in src/index.js

Issue Resolved

Fixes #930

index.html Outdated
</head>

<body>
<div id="app"></div>
<script>
<script defer>
Copy link
Member

@jjhampton jjhampton Mar 28, 2018

Choose a reason for hiding this comment

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

Not sure there's any benefit to using the defer attribute on the second Raven script tag, since it's an inline script w/o a src attribute. Per MDN, on usage of defer w/ script tags:

This attribute must not be used if the src attribute is absent (i.e. for inline scripts), in this case it would have no effect.

@@ -35,7 +35,6 @@
"react-scroll": "^1.5.4",
"react-scroll-up-button": "^1.5.9",
"react-select": "^1.0.0-rc.5",
"react-table": "^6.0.5",
Copy link
Member

Choose a reason for hiding this comment

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

Finally got rid of this dependency, nice cleanup.

import './shared/styles/react-select.global.css';
import './shared/styles/notifications.css';
Copy link
Member

@jjhampton jjhampton Mar 28, 2018

Choose a reason for hiding this comment

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

@kylemh Should we also remove the file /public/notification.css ?

Another thing I'm not too knowledgeable about (possibly due to my inexperience w/ Webpack) is why we have two index HTML files - a /public/index.html and root-level index.html. In the public file, I still see the older (non-defer) Raven scripts, and also <link rel="stylesheet" href="%PUBLIC_URL%/notification.css">. Might we want to apply some of this PR's changes there too? On my dev machine, running this branch, I'm still seeing a CSS network request made for notification.css.

Copy link
Member Author

Choose a reason for hiding this comment

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

After a bit of reading around @alexspence 's helpful comments from ejection within webpack related files, looks like we're serving everything relevant via public, and doing so on purpose (some kind of vulnerability).

I've updated public/index.html, removed public/notification.css, and deleted the ~/index.html to avoid further confusion.

Copy link
Member

@jjhampton jjhampton left a comment

Choose a reason for hiding this comment

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

LGTM, everything works after pulling the branch. Just had a couple questions, see comments.

Notice the use of %PUBLIC_URL% in the tag above.
It will be replaced with the URL of the `public` folder during the build.
Only files inside the `public` folder can be referenced from the HTML.
<meta name="theme-color" content="#47566b">
Copy link
Member

Choose a reason for hiding this comment

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

Nice update of this theme color! Looks nice.

Event is in the past, this can be re-added for future event if needed
@jjhampton jjhampton merged commit 08a608a into master Apr 21, 2018
@kylemh kylemh deleted the various-performance-fixes branch April 22, 2018 22:48
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.

Various Easy Performance Boosts
2 participants