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: Use static Reflect methods in nodeGlobalThis proxy #17696

Merged
merged 6 commits into from
Feb 9, 2023
Merged

fix: Use static Reflect methods in nodeGlobalThis proxy #17696

merged 6 commits into from
Feb 9, 2023

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Feb 8, 2023

  get(_target, prop, _receiver) {
    if (prop in nodeGlobals) {
      return nodeGlobals[prop];
    } else {
      return globalThis[prop];
    }
  },

For some reason, this block was getting confused, and when missing property (eg. RegExp) was asked for, globalThis[prop] pointed to the same proxied object, which then recursed infinitely.
One solution was to just use target[prop] instead, however for the sake of completeness, I changed all methods to use Reflect instead, as ownKeys, defineProperty and getOwnPropertyDescriptor already did that.

I cannot come up with an explicit test-case (didnt invest much time into it yet), but so far all current tests are passing and linked issue is fixed when trying repro locally.

Fixes #17678

@bartlomieju bartlomieju requested a review from dsherret February 8, 2023 20:45
@kamilogorek
Copy link
Contributor Author

One thing worth mentioning is that this issue does not occur when loading the module in-directly via createRequire, which may point in the direction of npm: specifiers doing some magic.

eg. this works just fine, even without this fix

import { createRequire } from "https://deno.land/std@0.177.0/node/module.ts";
const require = createRequire(import.meta.url);
const prettier = require("./prettier");

prettier.format('', {filename: '1.js'});

ext/node/01_node.js Outdated Show resolved Hide resolved
console.log(setTimeout);
delete nodeGlobalThis["setTimeout"];
console.log(nodeGlobalThis["setTimeout"]); // should be undefined
console.log(globalThis["setTimeout"]); // should be undefined
Copy link
Member

Choose a reason for hiding this comment

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

On main these both go to undefined. I'm not entirely sure if this one should, but that's the current behaviour

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good other than the behaviour change in delete.

@kamilogorek
Copy link
Contributor Author

Good catch, thanks! Updated

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@dsherret dsherret merged commit ef9b669 into denoland:main Feb 9, 2023
@kamilogorek kamilogorek deleted the node-global-reflect branch February 9, 2023 08:05
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.

prettier npm module fails with "Maximum call stack size exceeded"
2 participants