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

Isaacs/do not break on missing global process #297

Merged

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Aug 31, 2021

  • work on machines that don't have curl (or where, like my laptop apparently, curl is somehow borked)
  • work on node 16, where fs.writeFileSync needs a proper String or Buffer argument.
  • work on systems where process is unset or null.

This was occasionally causing my machine to spin on CPU indefinitely
while trying to download the Closure Compiler output.

It would perhaps be better to investigate what is wrong with curl on my
machine, but since Node has a pretty nice built-in HTTPS client, I
figured it'd be easier to just cut out the external system dependency
entirely.
fs.writeFileSync no longer stringifies arguments, instead throwing if it
receives something that is not a proper String or Buffer.
@isaacs
Copy link
Contributor Author

isaacs commented Aug 31, 2021

me: "What idiot made it look at process.version without any kind of guard anyway!?"

also me: #255

😂

(I'm j/k, of course, no hostility about bugs.)

fs.writeFileSync('browser-source-map-support.js', code);
fs.writeFileSync('amd-test/browser-source-map-support.js', code);
console.log('making request to google closure compiler');
var https = require('https');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered stashing the response in node_modules/.cache somewhere and only making the request if the input file changed since the last one, but figured that might be overkill?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think that we should replace this with a local build system anyways, so no worries for now ☺️

This is the last item preventing node-tap from being browser-friendly.
Since global.process is safely guarded in other instances where it is
used, this seems like an oversight.
@isaacs isaacs force-pushed the isaacs/do-not-break-on-missing-global-process branch from f63cbbe to dd868f6 Compare September 6, 2021 18:23
@isaacs
Copy link
Contributor Author

isaacs commented Sep 6, 2021

Updated to also not break if process.stderr is missing.

@LinusU LinusU merged commit 3ce709b into evanw:master Sep 9, 2021
@LinusU
Copy link
Collaborator

LinusU commented Sep 9, 2021

Released as 0.5.20 🚀

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.

None yet

2 participants