fix: dynamically require os for edge runtime compatibility#1262
Conversation
| var override = require('../utils/override'); | ||
|
|
||
| function getSystemLineBreak() { | ||
| var systemLinkBreak = '\n'; |
There was a problem hiding this comment.
Should it be named systemLineBreak instead?
There was a problem hiding this comment.
sure should. I took the quick path and used github's editor to make the changes, and was counting on CI to catch any slips.
There was a problem hiding this comment.
👍 let's see if it catches anything
|
Thanks @shellscape - just one small nitpick. Let me know if I got it wrong! |
|
alright a buncha stuff. I'll clean this up in a few |
|
Very weird, however the |
|
That's interesting. So in test env under Windows it falls back to '\n' which causes tests to fail, right? |
|
Bingo. I'm looking into what vows is doing so I can get the correct check+polyfill for both tests and non-tests. |
|
Gah. needs approval to run CI again. |
|
It's odd that I have to approve it every time. Is there a setting to override it? |
|
Actually not sure haha. Before we merge, I'd like @cprussin to put eyes on this since they were first to report. |
|
oh awesome, thanks so much @shellscape . Yeah, as far as I know this will fix the edge runtime compat for this lib. Thanks again. |
|
So we are fine to merge it, right? |
|
I believe so! |
|
Done. I'll do a patch release too. |
|
Really appreciate the attention to this one 🙏 |
|
It's released in clean-css 5.3.3. |
|
Amazing, thank you! |
This essentially makes
require('os')dynamic for edge worker runtime compatibility (shellscape/jsx-email#82 (comment)). The require ofosas it currently is in master can be polyfilled away but requires use knowledge about bundlers and the correct way to bundle the package, if used directly, and any consumers of clean-css if used as a transitive dependency. Bundling is a minefield for most folks, and this should provide some relief.