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

either preload OR inline CSS #6009

Conversation

thescientist13
Copy link
Contributor

@thescientist13 thescientist13 commented Jun 19, 2018

Resolves #6008 by either inlining styles OR prefetching them, but not both.

Note: There were modifications to my local yarn.lock file (not committed here) but was wondering if maybe it should be (in a different PR maybe?). Not sure if maybe a recent contribution missed regenerating the local file? Here's what I see locally

```shell $ git diff yarn.lock diff --git a/yarn.lock b/yarn.lock index df3df885..76f10051 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3033,7 +3033,7 @@ caniuse-db@^1.0.30000529, caniuse-db@^1.0.30000634, caniuse-db@^1.0.30000639: version "1.0.30000856" resolved "https://registry.yarnpkg.com/caniuse-db/-/caniuse-db-1.0.30000856.tgz#fbebb99abe15a5654fc7747ebb5315bdfde3358f"

-caniuse-lite@^1.0.30000805, caniuse-lite@^1.0.30000844:
+caniuse-lite@^1.0.30000792, caniuse-lite@^1.0.30000805, caniuse-lite@^1.0.30000844:
version "1.0.30000856"
resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30000856.tgz#ecc16978135a6f219b138991eb62009d25ee8daa"

@@ -3937,7 +3937,7 @@ core-js@^1.0.0:
version "1.2.7"
resolved "https://registry.yarnpkg.com/core-js/-/core-js-1.2.7.tgz#652294c14651db28fa93bd2d5ff2983a4f08c636"

-core-js@^2.4.0, core-js@^2.4.1, core-js@^2.5.0, core-js@^2.5.7:
+core-js@^2.4.0, core-js@^2.4.1, core-js@^2.5.0, core-js@^2.5.3, core-js@^2.5.7:
version "2.5.7"
resolved "https://registry.yarnpkg.com/core-js/-/core-js-2.5.7.tgz#f972608ff0cead68b841a16a932d0b183791814e"

@@ -4982,7 +4982,7 @@ ejs@^2.4.1:
version "2.6.1"
resolved "https://registry.yarnpkg.com/ejs/-/ejs-2.6.1.tgz#498ec0d495655abc6f23cd61868d926464071aa0"

-electron-to-chromium@^1.2.7, electron-to-chromium@^1.3.47:
+electron-to-chromium@^1.2.7, electron-to-chromium@^1.3.30, electron-to-chromium@^1.3.47:
version "1.3.48"
resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.3.48.tgz#d3b0d8593814044e092ece2108fc3ac9aea4b900"

@@ -10673,14 +10673,6 @@ parse-json@^4.0.0:
error-ex "^1.3.1"
json-parse-better-errors "^1.0.1"

-parse-latin@^3.1.0:

parse-latin@^4.0.0:
version "4.1.1"
resolved "https://registry.yarnpkg.com/parse-latin/-/parse-latin-4.1.1.tgz#3a3edef405b2d5dce417b7157d3d8a5c7cdfab1d"
@@ -12751,11 +12743,11 @@ retext-english@^3.0.0:
parse-english "^4.0.0"
unherit "^1.0.4"

-retext-latin@^1.0.0:

  • parse-latin "^3.1.0"
  • parse-latin "^4.0.0"
    unherit "^1.0.4"

retext-smartypants@^3.0.0:
@@ -12765,9 +12757,9 @@ retext-smartypants@^3.0.0:
nlcst-to-string "^2.0.0"
unist-util-visit "^1.0.0"

-retext-stringify@^1.0.0:

@@ -14727,19 +14719,6 @@ unified@^4.1.1:
trough "^1.0.0"
vfile "^1.0.0"

-unified@^5.0.0:

unified@^6.0.0, unified@^6.1.4, unified@^6.1.5:
version "6.2.0"
resolved "https://registry.yarnpkg.com/unified/-/unified-6.2.0.tgz#7fbd630f719126d67d40c644b7e3f617035f6dba"

</details>

@thescientist13 thescientist13 changed the title either preload OR inline either preload OR inline CSS Jun 19, 2018
headComponents.unshift(
<style
type="text/css"
data-href={urlJoin(pathPrefix, style.name)}
Copy link
Contributor Author

@thescientist13 thescientist13 Jun 19, 2018

Choose a reason for hiding this comment

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

Not sure if data-href should be removed under this condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

No that should be there still. Is there so webpack knows the styles have already been loaded so won't try again.

Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

Nice, thanks @thescientist13 👍

@m-allanson
Copy link
Contributor

A separate PR for updating the yarn.lock file would be splendid 👌, if you get a chance to do that.

@m-allanson m-allanson changed the base branch from v2 to master June 20, 2018 15:22
@m-allanson m-allanson changed the base branch from master to v2 June 20, 2018 15:23
@thescientist13
Copy link
Contributor Author

@m-allanson / @KyleAMathews
Based on the build failure, should I update Jest snapshots?

@thescientist13
Copy link
Contributor Author

thescientist13 commented Jun 20, 2018

@m-allanson
Looks like the yarn.lock file was updated as part of this PR. Running yarn bootstrap on latest version of the v2 branch shows now diffs on yarn.lock, so that's all set now! 👍

@m-allanson
Copy link
Contributor

m-allanson commented Jun 21, 2018

The appveyor issue is tracked here: #5725, If you have access to a Windows machine and want to investigate further that'd be very helpful!

I have one slightly annoying request (I should have noticed this earlier) - could you close this PR and re-open it against master branch? We merged v2 into master over the weekend and will be deleting the v2 branch once some extra git admin has been handled.

@KyleAMathews KyleAMathews merged commit 022ed07 into gatsbyjs:v2 Jun 21, 2018
@KyleAMathews
Copy link
Contributor

Thanks!

KyleAMathews pushed a commit that referenced this pull request Jun 21, 2018
* either preload OR inline

* Make sure all assets added
@KyleAMathews
Copy link
Contributor

Just ended up cherry picking this change :-) Gotta kill that v2 branch later haha

KyleAMathews pushed a commit that referenced this pull request Jun 21, 2018
* either preload OR inline

* Make sure all assets added
@thescientist13
Copy link
Contributor Author

congrats on the merge of v2 into master!! 🎉

@thescientist13 thescientist13 deleted the bug/issue-6008-dont-preload-inlined-css branch June 23, 2018 15:18
@thescientist13
Copy link
Contributor Author

@m-allanson
I don't have a windows machine, but I am comfortable with setting up Windows via a VM (like VirtualBox). Would that be sufficient to reproduce #5725 ?

@thescientist13
Copy link
Contributor Author

oh, looks like it may have already been solved? 👀

your team is quick! 🏃

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.

3 participants