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 util.global #1441

Merged
merged 2 commits into from Jul 13, 2020
Merged

Fix util.global #1441

merged 2 commits into from Jul 13, 2020

Conversation

@dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Jul 13, 2020

An alternative to #1363, attempting to fix the problem at the util.global level as proposed in #1221 (comment).

Reverts #1363 and #1440, with the latter not being needed anymore. See also: #1363 (comment)

@dcodeIO dcodeIO requested a review from alexander-fenster Jul 13, 2020
@alexander-fenster
Copy link
Contributor

@alexander-fenster alexander-fenster commented Jul 13, 2020

Thank you @dcodeIO, you definitely have more experience with bundlers than I do :)

Loading

@alexander-fenster alexander-fenster merged commit 742b8dc into master Jul 13, 2020
9 checks passed
Loading
@alexander-fenster alexander-fenster deleted the fix-global branch Jul 13, 2020
@dcodeIO
Copy link
Member Author

@dcodeIO dcodeIO commented Jul 13, 2020

Sorry for second-guessing btw :). If I have any bundler experience, then it is in preventing them from functioning as intended. As lost as everybody else otherwise.

Loading

@evgenius
Copy link

@evgenius evgenius commented Jul 14, 2020

Object.prototype.toString.call(global) === "[object global]" seems to be too strict.
Now protobufjs fails to initialize in the Jest environment since Jest modifies global. See https://github.com/facebook/jest/blob/d7f3427/packages/jest-environment-node/src/index.ts#L30

Loading

@dcodeIO
Copy link
Member Author

@dcodeIO dcodeIO commented Jul 14, 2020

Hmm, more forgiving checks there could be

Object.prototype.toString.call(global.process) === "[object process]"

if process remains unmodified, and/or

global.setTimeout === setTimeout

to just validate that it's at least somewhat an actual global object.

Loading

@dcodeIO
Copy link
Member Author

@dcodeIO dcodeIO commented Jul 14, 2020

A more general question would be what Jest is actually trying to test there. Does a Jest test suite automatically assume that it's testing in a node environment, or may it also test client code while filling in browser APIs? If the latter is possible, protobuf.js cannot decide on its own but will need some sort of flag to indicate, i.e. setting util.isNode manually.

Loading

@evgenius
Copy link

@evgenius evgenius commented Jul 14, 2020

How about changing the order?

util.isNode = typeof global !== "undefined"
           && global.process
           && global.process.versions
           && global.process.versions.node;
util.global = util.isNode && global 
           || typeof window !== "undefined" && window
           || typeof self   !== "undefined" && self
           || this;

If something tries to pretend it's Node by having versions.node, I think it should be just allowed to do so. What do you think?

Loading

@dcodeIO
Copy link
Member Author

@dcodeIO dcodeIO commented Jul 14, 2020

That looks good as well, just needs another check && global before accessing global.process in case it's null.

Loading

@evgenius
Copy link

@evgenius evgenius commented Jul 14, 2020

Loading

@alexander-fenster
Copy link
Contributor

@alexander-fenster alexander-fenster commented Jul 14, 2020

Folks also suggested #1444 as a possible fix to the strict [object global]. I must admit I lost track of all the possible environments and frameworks this code should work in :) But I would really appreciate if all the fixes for "does not work for me" cases try to update the test suite if possible, otherwise we will keep breaking stuff (with good intentions of course).

Loading

@dcodeIO dcodeIO mentioned this pull request Jul 14, 2020
taylorcode added a commit to taylorcode/protobuf.js that referenced this issue Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants