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.

Sign up for GitHub

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

Jump to bottom

v4 fails to import https resources on macOS, works in v3 #3693

Closed
joeyparrish opened this issue Feb 19, 2022 · 10 comments · Fixed by #3716
Closed

v4 fails to import https resources on macOS, works in v3 #3693

joeyparrish opened this issue Feb 19, 2022 · 10 comments · Fixed by #3716
Labels
bug

Comments

@joeyparrish
Copy link
Contributor

To reproduce:

@import (css, inline) "https://fonts.googleapis.com/css?family=Roboto";

Current behavior:
Fails with ECONNRESET on macOS with less v4.1.2, but succeeds with less v3.13.1. Doesn't fail on Linux or Windows. Reproducible in GitHub Actions as well as on our private mac hardware.

Expected behavior:
Should be able to fetch https resources equally on all platforms.

Environment information:

  • less version: 4.1.2, but not 3.13.1
  • nodejs version: 17.5.0
  • operating system: macOS
This was referenced Feb 19, 2022
Closed
Merged
joeyparrish added a commit to shaka-project/shaka-player that referenced this issue Feb 22, 2022
@joeyparrish
build: Downgrade less (#3982)
a67cb6e 
Downgrade less to v3.  v4 is failing on macOS for some reason.  See
less/less.js#3693

This also makes some less/CSS changes that are useful for future
upgrades:

 - wrap all calculations in calc(), which is required in less v4
 - remove unneeded @transparent variable

Finally, this fixes an erroneous error message that said "extern
generation failed" instead of "CSS compilation failed".

Closes #3981
@mhnaeem
Copy link

mhnaeem commented Mar 10, 2022 •
edited
Loading

Facing this same issue on my mac as well

Possibly related issue: nodejs/node#27916

@joeyparrish joeyparrish mentioned this issue Mar 15, 2022
Merged
@joeyparrish
Copy link
Contributor Author

I've traced it to needle, a new dependency of less.js. They remove their error listener after closing the connection, but nodejs fires an error after that.

@joeyparrish joeyparrish mentioned this issue Mar 15, 2022
Closed
@joeyparrish
Copy link
Contributor Author

See tomas/needle#391

joeyparrish added a commit to joeyparrish/needle that referenced this issue Mar 15, 2022
@joeyparrish
fix: Silence ECONNRESET errors after connection close
86b2c2f 
See also less/less.js#3693

Fixes tomas#391
@joeyparrish joeyparrish mentioned this issue Mar 15, 2022
Merged
joeyparrish added a commit to joeyparrish/less.js that referenced this issue Mar 15, 2022
@joeyparrish
fix: Use pre-release fix for needle
aef9a5c 
See also:
 - shaka-project/shaka-player#3981
 - less#3693
 - tomas/needle#392
 - nodejs/node#27916
joeyparrish added a commit to joeyparrish/less.js that referenced this issue Mar 15, 2022
@joeyparrish
fix: Use pre-release fix for needle
e7ac948 
See also:
 - shaka-project/shaka-player#3981
 - less#3693
 - tomas/needle#392
 - nodejs/node#27916
joeyparrish added a commit to joeyparrish/less.js that referenced this issue Mar 15, 2022
@joeyparrish
fix: Use pre-release fix for needle
4fb5384 
See also:
 - shaka-project/shaka-player#3981
 - less#3693
 - tomas/needle#392
 - nodejs/node#27916
joeyparrish added a commit to shaka-project/shaka-player that referenced this issue Mar 15, 2022
@joeyparrish
chore(deps): Upgrade less to v4 (prerelease) (#4028)
f5f5177 
In #3991, I changed the syntax of our colors to a modern rgba syntax.
For example, rgba(255, 255, 255, 0.85) would become rgba(255 255 255 /
85%). However, less v3 seems not to understand that properly, and
performs division on the last two parts, resulting in output of
rgba(255 255 3%), which is indeed invalid.

This fixes the issue by upgrading to less v4, which understands the
new rgba syntax and leaves it alone. The output for that will now
match the input.

To work around an issue with less v4, this uses a prerelease version
with a fix for less/less.js#3693 . See also
tomas/needle#391

This doesn't affect any release branches, since #3991 hasn't been
cherry-picked.

Closes #4027
@iChenLei
Copy link
Member

Thanks for bug report, wait upstream deps to fix it.

@joeyparrish
Copy link
Contributor Author

A new version of needle is available upstream. As soon as I can write a regression test for less.js, I'll put up a PR to upgrade needle.

@joeyparrish
Copy link
Contributor Author

There seems to be a timing component to the issue. The simplest possible regression test doesn't reproduce the issue, but the full set of less files in shaka-project/shaka-player does.

Also, the issue does not appear in node v12, but does appear in v14, v16, and v17.

If I can't modify an existing inline-https test to fail on mac without the fix, I may give up on a regression test. I'd prefer not to, though.

joeyparrish added a commit to joeyparrish/less.js that referenced this issue Apr 20, 2022
@joeyparrish
fix: Fix https failures on macOS
3608a27 
The issue was traced upstream to needle, and resolved in:
 - tomas/needle#392
 - tomas/needle#394
 - tomas/needle#396
 - tomas/needle#398

Closes less#3693
@joeyparrish
Copy link
Contributor Author

If I strip away some of shaka-player's less files, the repro rate goes down steadily, from 100% repro with all of them, down to 0% for just an https inline import statement.

I can't find a simple, non-project-specific repro case for a regression test, so I'm giving up.

I can confirm, though, that with the needle upgrade, shaka-player's less build works 100% of the time on our mac with node 17.

@joeyparrish joeyparrish mentioned this issue Apr 20, 2022
Merged
3 tasks
iChenLei pushed a commit that referenced this issue Apr 22, 2022
@joeyparrish
fix: Fix https failures on macOS (#3716)
1eafc06 
The issue was traced upstream to needle, and resolved in:
 - tomas/needle#392
 - tomas/needle#394
 - tomas/needle#396
 - tomas/needle#398

Closes #3693
@joeyparrish
Copy link
Contributor Author

Thanks so much for reviewing and merging. Looking forward to seeing the fix in an official release!

lumburr pushed a commit to lumburr/less.js that referenced this issue Apr 28, 2022
@joeyparrish @lumburr
fix: Fix https failures on macOS (less#3716)
97f7082 
The issue was traced upstream to needle, and resolved in:
 - tomas/needle#392
 - tomas/needle#394
 - tomas/needle#396
 - tomas/needle#398

Closes less#3693
@joeyparrish
Copy link
Contributor Author

Any chance we could get an updated release for less.js so we can stop using a fork?

@joeyparrish
Copy link
Contributor Author

Less v4.2.0 seems to include the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Fix https failures on macOS joeyparrish/less.js
3 participants
@joeyparrish @iChenLei @mhnaeem