Comments (29)
addyosmani
commented on August 17, 2024
Hmm. I haven't tested on Windows yet but this looks very curious.
Could you let me know if you're able to get this sample using Critical running for you correctly on Windows https://github.com/addyosmani/critical-path-css-demo?
Alternatively, just verifying the module works in your environment would be of great help. I'm trying to determine if this is a test harness issue, fs, penthouse or straight-out something silly I'm doing in the module.
from critical.
XhmikosR
commented on August 17, 2024
Sure thing. I'll test things out when I'm back.
On Jul 5, 2014 12:40 PM, "Addy Osmani" [email protected] wrote:
Hmm. I haven't tested on Windows yet but this looks very curious.
Could you let me know if you're able to get this sample using Critical
running for you correctly on Windows
https://github.com/addyosmani/critical-path-css-demo?Alternatively, just verifying the module works in your environment would
be of great help. I'm trying to determine if this is a test harness issue,
fs, penthouse or straight-out something silly I'm doing in the module.—
Reply to this email directly or view it on GitHub
#14 (comment).
from critical.
addyosmani
commented on August 17, 2024
Appreciate it!
from critical.
XhmikosR
commented on August 17, 2024
I can't even install the dependencies for the demo... too many errors.
At first I thought it's because of too long filenames. I even moved the repo in C:\ but I still can't install the deps.
from critical.
XhmikosR
commented on August 17, 2024
Full log for the demo https://gist.githubusercontent.com/XhmikosR/2ce380c87e80a8de980f/raw/critical-demo-npm-install.log
from critical.
addyosmani
commented on August 17, 2024
That's crazy. I wonder if the same issues occur with running https://github.com/pocketjoso/penthouse standalone vs. my module. I don't have a windows box handy but will try to debug this issue next week and get to the bottom of it.
from critical.
XhmikosR
commented on August 17, 2024
Nope, penthouse works fine by doing npm install && npm test...
from critical.
addyosmani
commented on August 17, 2024
Interesting. There's something silly I'm doing in this module then. Looking into it.
from critical.
addyosmani
commented on August 17, 2024
@XhmikosR pushed some potential fixes to master. Could you let me know if there's any difference in test output?
from critical.
XhmikosR
commented on August 17, 2024
Sure, I'll try when I'm back☺
On Jul 7, 2014 2:29 AM, "Addy Osmani" [email protected] wrote:
@XhmikosR https://github.com/XhmikosR pushed some potential fixes to
master. Could you let me know if there's any difference in test output?—
Reply to this email directly or view it on GitHub
#14 (comment).
from critical.
XhmikosR
commented on August 17, 2024
C:\Users\xmr\Desktop\critical>npm test
> [email protected] test C:\Users\xmr\Desktop\critical
> mocha test.js --timeout 15000
|
.././
2 passing (3s)
6 failing
1) generates critical-path CSS successfully:
Uncaught AssertionError: false == true
at C:\Users\xmr\Desktop\critical\test.js:33:9
at C:\Users\xmr\Desktop\critical\index.js:60:15
at C:\Users\xmr\Desktop\critical\node_modules\mocha\node_modules\glob\node_modules\graceful-fs\graceful-fs.js:104:5
at Object.oncomplete (fs.js:107:15)
2) generates minified critical-path CSS successfully:
Uncaught AssertionError: false == true
at C:\Users\xmr\Desktop\critical\test.js:47:9
at C:\Users\xmr\Desktop\critical\index.js:63:13
at ChildProcess.<anonymous> (C:\Users\xmr\Desktop\critical\node_modules\penthouse\lib\index.js:43:13)
at ChildProcess.emit (events.js:98:17)
at Process.ChildProcess._handle.onexit (child_process.js:809:12)
3) generates critical-path CSS without writing to disk:
Uncaught AssertionError: false == true
at C:\Users\xmr\Desktop\critical\test.js:60:9
at C:\Users\xmr\Desktop\critical\index.js:63:13
at ChildProcess.<anonymous> (C:\Users\xmr\Desktop\critical\node_modules\penthouse\lib\index.js:43:13)
at ChildProcess.emit (events.js:98:17)
at Process.ChildProcess._handle.onexit (child_process.js:809:12)
4) inlines critical-path CSS successfully:
Uncaught AssertionError: false == true
at C:\Users\xmr\Desktop\critical\test.js:72:7
at C:\Users\xmr\Desktop\critical\index.js:92:9
at C:\Users\xmr\Desktop\critical\node_modules\mocha\node_modules\glob\node_modules\graceful-fs\graceful-fs.js:104:5
at Object.oncomplete (fs.js:107:15)
5) inlines critical-path CSS without writing to disk:
Uncaught AssertionError: false == true-
6) inlines and minified critical-path CSS:s:83:7
Uncaught AssertionError: false == truex.js:95:9
at C:\Users\xmr\Desktop\critical\test.js:96:7
at C:\Users\xmr\Desktop\critical\index.js:92:9mocha\node_modules\glob\node
at C:\Users\xmr\Desktop\critical\node_modules\mocha\node_modules\glob\node_modules\graceful-fs\graceful-fs.js:104:5
at Object.oncomplete (fs.js:107:15)
npm ERR! Test failed. See above for more details.
npm ERR! not ok code 0
from critical.
addyosmani
commented on August 17, 2024
Thanks to Sindre we have a fresh rewrite now in master. Any luck now?
from critical.
XhmikosR
commented on August 17, 2024
I tried a couple hours ago after those patches and I still get the same
failures...
On Jul 8, 2014 11:58 PM, "Addy Osmani" [email protected] wrote:
Thanks to Sindre we have a fresh rewrite now in master. Any luck now?
—
Reply to this email directly or view it on GitHub
#14 (comment).
from critical.
addyosmani
commented on August 17, 2024
This is really weird. @sindresorhus I don't suppose you have any hints that would be worth considering here, would you?
from critical.
sindresorhus
commented on August 17, 2024
@XhmikosR run the test from master now and paste in the output ;)
Also look at the terminal output from installing dependencies. The phantomjs install might have failed.
from critical.
XhmikosR
commented on August 17, 2024
Sure, I'll do it first thing tomorrow.
On Wed, Jul 9, 2014 at 12:54 AM, Sindre Sorhus [email protected]
wrote:
@XhmikosR https://github.com/XhmikosR run the test from master now and
paste in the output ;)—
Reply to this email directly or view it on GitHub
#14 (comment).
from critical.
XhmikosR
commented on August 17, 2024
https://gist.github.com/XhmikosR/8b69a2a39841f9edfd05
from critical.
XhmikosR
commented on August 17, 2024
With LF forced for the test files, I get 3/6 passed BTW.
from critical.
addyosmani
commented on August 17, 2024
I wonder if it's more sensible for our API tests to just test against the output of each method directly rather than writing to disk and comparing the read-in input. Do any of our non i/o tests pass on Windows?
from critical.
XhmikosR
commented on August 17, 2024
With this patch and LF forced for all the repository files I get 3 failing tests.
C:\Users\xmr\Desktop\critical>npm test
> [email protected] test C:\Users\xmr\Desktop\critical
> mocha test.js --timeout 15000
|
..../
5 passing (44s)
3 failing
1) generates critical-path CSS successfully:
Error: timeout of 15000ms exceeded
at null.<anonymous> (C:\Users\xmr\Desktop\critical\node_modules\mocha\lib\runnable.js:139:19)
at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)
2) generates minified critical-path CSS successfully:
Error: timeout of 15000ms exceeded
at null.<anonymous> (C:\Users\xmr\Desktop\critical\node_modules\mocha\lib\runnable.js:139:19)
at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)
3) generates critical-path CSS without writing to disk:
Uncaught AssertionError: "body {\n padding-top: 20px;\n padding-bott
om: 20px;\n}\n\n\n.header,\n.marketing{\n padding-left: 15px;\n padding-r
=== "body {\r\n padding-top: 20px;\r\n padding-bottom: 20px;\r\n}\r\n\r\n\
r\n.header,\r\n.marketing{\r\n padding-left: 15px
+ expected - actual
+body {<CR>
-body {
- padding-top: 20px;
- padding-bottom: 20px;
-}
+ padding-top: 20px;<CR>
+ padding-bottom: 20px;<CR>
-.header,
-.marketing{
- padding-left: 15px;
- padding-right: 15px;
-}
+}<CR>
+<CR>
-.header {
- border-bottom: 1px solid #e5e5e5;
-}
+<CR>
+.header,<CR>
-.header h3 {
- margin-top: 0;
- margin-bottom: 0;
/- line-height: 40px;
- padding-bottom: 19px;
-}
+.marketing{<CR>
+ padding-left: 15px;<CR>
-.jumbotron {
- text-align: center;
- border-bottom: 1px solid #e5e5e5;
-}
+ padding-right: 15px;<CR>
-.jumbotron .btn {
- font-size: 21px;
- padding: 14px 24px;
-}
+}<CR>
+<CR>
-.marketing {
- margin: 40px 0;
-}
+<CR>
+.header {<CR>
-@media screen and (min-width: 768px) {
- .container {
- max-width: 730px;
- }
+ border-bottom: 1px solid #e5e5e5;<CR>
-
- .header,
- .marketing{
- padding-left: 0;
- padding-right: 0;
- }
+}<CR>
+<CR>
+<CR>
+ margin-top: 0;<CR>-
+ margin-bottom: 0;<CR>
+ line-height: 40px;<CR>
+ padding-bottom: 19px;<CR>
+}<CR>
+<CR>
+<CR>
+.jumbotron {<CR>
+ text-align: center;<CR>
+ border-bottom: 1px solid #e5e5e5;<CR>
+}<CR>
+<CR>
+.jumbotron .btn {<CR>
+ font-size: 21px;<CR>
+ padding: 14px 24px;<CR>
+}<CR>
+<CR>
+<CR>
+.marketing {<CR>
+ margin: 40px 0;<CR>
+}<CR>
+<CR>
+<CR>
+@media screen and (min-width: 768px) {<CR>
+ .container {<CR>
+ max-width: 730px;<CR>
+ }<CR>
\+<CR>
+ <CR>
+ .header,<CR>
+ .marketing{<CR>
+ padding-left: 0;<CR>
+ padding-right: 0;<CR>
+ }<CR>
+<CR>
+ <CR>
+ .header {<CR>
+ margin-bottom: 30px;<CR>
+ }<CR>
+ <CR>
+ <CR>
+ .jumbotron {<CR>
+ border-bottom: 0;<CR>
+ }<CR>
+}<CR>
-
- .header {
- margin-bottom: 30px;
- }
-
-
- .jumbotron {
- border-bottom: 0;
- }
-}
at C:\Users\xmr\Desktop\critical\test.js:28:16
at C:\Users\xmr\Desktop\critical\index.js:75:21
at C:\Users\xmr\Desktop\critical\node_modules\mocha\node_modules\glob\node_modules\graceful-fs\graceful-fs.js:104:5
at Object.oncomplete (fs.js:107:15)
npm ERR! Test failed. See above for more details.
npm ERR! not ok code 0
So I'd say we should have this merged.
from critical.
addyosmani
commented on August 17, 2024
I've been testing the module on Windows 8 today and ran into the same issues @XhmikosR mentions above. In addition, even if we're not just talking about the unit tests, the demo app using this module also seems to silently fail (builds, but critical path CSS is not inlined).
Something like this however does work (pardon Notepad):
In contrast, I was able to get the penthouse test working locally on windows with no issues.
from critical.
addyosmani
commented on August 17, 2024
Working on fixes. Have 4 tests passing now. Just a few more to go :)
from critical.
XhmikosR
commented on August 17, 2024
Cool, almost there 3/4 passing. One thing I notice with the tests branch is
that fixture\styles\critical.css is modified and is different so that must
be the one failing test.
Also the isWindows variable seem to be only used once, it's your call if
you want to keep it. var lineBreak = process.platform == 'win32' ? /\r\n/g : /\n/g; would work too.
On Sun, Jul 13, 2014 at 11:51 PM, Addy Osmani [email protected]
wrote:
Working on fixes. Have 4 tests passing now. Just a few more to go :)
—
Reply to this email directly or view it on GitHub
#14 (comment).
from critical.
addyosmani
commented on August 17, 2024
Good catch. We could indeed just simplify that further. I think you're right that the critical.css file might just be the one that needs correcting. Most of the tests are failing on inlining atm.
from critical.
sindresorhus
commented on August 17, 2024
var lineBreak = process.platform == 'win32' ? /\r\n/g : /\n/g;would work too.
You want require('os').EOL.
from critical.
addyosmani
commented on August 17, 2024
Hah! I didn't realize that was supported :)
from critical.
XhmikosR
commented on August 17, 2024
Didn't know this existed either, thanks.
from critical.
bezoerb
commented on August 17, 2024
All green now on windows even though the newlines had to be fixed in inline-critical
from critical.
bezoerb
commented on August 17, 2024
Tests still green on windows. Closing this for now.
from critical.
Related Issues (20)
- Remove query string from stylesheet path
- Print stylesheet unexpectedly gets `onload` attribute HOT 3
- Generated CSS is big HOT 8
- 'noscript)>[rel="stylesheet"]' is not a valid selector HOT 6
- Generate ciritical for multiple pages HOT 2
- Request: Add option to disable link media attribute wrapping HOT 1
- Rules within media queries duplicate when using multiple dimensions HOT 2
- Lots of css generated from below the fold
- Does not take redirects into account when looking for relative CSS HOT 2
- Does not take the <base HTML tag into account when looking for relative CSS HOT 7
- dimensions don't work HOT 2
- Error: Failed to launch the browser process! HOT 3
- typescript support ? HOT 1
- Add the option of a prefix path HOT 3
- On @container queries
- PageSpeed Insights consider CSS not deferred HOT 1
- Allowing internal functions to be imported HOT 3
- [✨] feat: Add the option of pass plain css HOT 2
- Several warnings appear when installing the latest version of critical HOT 3
- Resource Utilization
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from critical.