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

fix: updated isNode check (#1221) #1363

Merged
merged 2 commits into from May 29, 2020
Merged

fix: updated isNode check (#1221) #1363

merged 2 commits into from May 29, 2020

Conversation

diegoblattner
Copy link
Contributor

@alexander-fenster alexander-fenster merged commit 5564e7b into protobufjs:master May 29, 2020
@dcodeIO
Copy link
Member

dcodeIO commented May 29, 2020

Haven't checked and might be a false alarm, but referencing process directly as proposed here might cause the bundler to pull in a process shim, increasing distribution size, and potentially breaking browser builds due to assuming they'd run under node. Thought I mention :)

@alexander-fenster
Copy link
Contributor

Thanks @dcodeIO, let me check this. This is basically why we decided to merge stuff on Fridays and let it bake for a while before we make an actual release next week :) I'll update the issue and/or send a fix if needed.

@alexander-fenster
Copy link
Contributor

So, several things here:

  • it indeed includes a shim for process now (I checked with webpack). It pulls in the whole ./node_modules/process/browser.js which adds 1.6K of code when minimized. That's sad.
  • it may or may not break some builds indeed (if someone just checks for typeof process !== "undefined" to check if it's Node or not, this code may stop working within the bundle), even though this check is actually wrong;
  • adding a shim for process can be disabled in the bundler config (e.g. webpack.config.js or similar).
  • there is an NPM module called browser-or-node (28.5k weekly downloads) that actually implements isNode in the same way as introduced in this PR.

My personal opinion here is that it's better to fix the code for users who actually reported they have problems (in #1221), and then fix it once more if the fix actually breaks someone else's code (it definitely will, obligatory xkcd). Folks who really care about the extra 1.6K of minimized code will be able to configure their bundler to remove this shim.

@alexander-fenster
Copy link
Contributor

...but the bad thing is that it actually breaks npm run make (ReferenceError: process is not defined), so yes we'll either need to rollback or figure out a different way to fix it.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 13, 2020

The util.global alias was intentionally introduced to avoid the shim bundling issues we are having now, so perhaps #1221 (comment) is a better fix than this one. Like, util.global.process being missing appears to be more of a symptom of an underlying issue with what we alias into util.global. If the fix proposed within the linked comment doesn't introduce any immediate issues, I recommend to revert this PR and fix util.global as proposed instead.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 13, 2020

Perhaps with an additional safeguard there:

function checkNodeGlobal(global) {
  if (global && global.process) return global; // or whatever reasonably identifies node
  return null;
}

// global object reference
util.global = typeof global !== "undefined" && checkNodeGlobal(global)
           || typeof window !== "undefined" && window
           || typeof self   !== "undefined" && self
           || this; // eslint-disable-line no-invalid-this

@dcodeIO dcodeIO mentioned this pull request Jul 13, 2020
taylorcode pushed a commit to taylorcode/protobuf.js that referenced this pull request Oct 16, 2020
* Fixes protobufjs#1221 when used within SSR.
`process` is a global variable since node [0.1.7](https://nodejs.org/api/globals.html#globals_process) and versions since [0.2.0](https://nodejs.org/api/process.html#process_process_versions).

Co-authored-by: Alexander Fenster <fenster@google.com>
This was referenced May 20, 2022
@github-actions github-actions bot mentioned this pull request Jul 8, 2022
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.

util.isNode / Error: not supported
3 participants