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

Object.getOwnPropertyDescriptor(global, 'performance').writable is no longer true in v18.19.0 #51612

Open
merceyz opened this issue Jan 31, 2024 · 3 comments

Comments

@merceyz
Copy link
Member

merceyz commented Jan 31, 2024

Version

v18.19.0

Platform

Linux unknown 5.15.0-92-generic #102-Ubuntu SMP Wed Jan 10 09:33:48 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

bootstrap

What steps will reproduce the bug?

console.assert(
	Object.getOwnPropertyDescriptor(global, 'performance').writable === true,
	'Expected global.performance to be writable',
);

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

global.performance should remain writable between minor versions.

What do you see instead?

Assertion failed: Expected global.performance to be writable

Additional information

Ref #51048.
Ref jestjs/jest#14741.

git bisect points to 6466acb.

Tests started failing in https://github.com/yarnpkg/berry/tree/a8857df67ba769e265b49542af9c31d9f05ee681 which uses jest@28, minimal reproduction for that:

cd $(mktemp -d)
npm init -y
npm i jest@28
echo "jest.useFakeTimers(); it('foo', () => {})" > jest.test.js
npx jest jest.test.js

which produces this error:

 FAIL  ./jest.test.js
  ● Test suite failed to run

    TypeError: Cannot assign to read only property 'performance' of object '[object global]'

    > 1 | jest.useFakeTimers(); it('foo', () => {})
        |                     ^
      2 |

      at hijackMethod (node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:946:32)
      at Object.install (node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:1733:17)
      at FakeTimers.useFakeTimers (node_modules/@jest/fake-timers/build/modernFakeTimers.js:110:36)
      at Object.<anonymous> (jest.test.js:1:21)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.237 s, estimated 1 s
Ran all test suites matching /jest.test.js/i.
@merceyz merceyz changed the title global.performance is no longer writable in v18.19.0 Object.getOwnPropertyDescriptor('global.performance').writable is no longer true in v18.19.0 Jan 31, 2024
@merceyz merceyz changed the title Object.getOwnPropertyDescriptor('global.performance').writable is no longer true in v18.19.0 Object.getOwnPropertyDescriptor(global, 'performance').writable is no longer true in v18.19.0 Jan 31, 2024
@aduh95
Copy link
Contributor

aduh95 commented Feb 3, 2024

@nodejs/lts

@targos
Copy link
Member

targos commented Feb 5, 2024

Is it supposed to be true ?

@kjleitz
Copy link

kjleitz commented Apr 16, 2024

From a skim, yes, it looks like it used to be true, and the identified commit (6466acb) omitted the writable property from the options passed to ObjectDefineProperty, which disabled it. I'm not super familiar with Node development, but if the behavior of ObjectDefineProperty() mirrors Object.defineProperty(), then writable defaults to false; an omission of that key will disable writability.

Was:

defineReplacableAttribute(globalThis, 'performance',
                          perf_hooks.performance);

// ...

function defineReplacableAttribute(target, name, value) {
  ObjectDefineProperty(target, name, {
    __proto__: null,
    writable: true,
    enumerable: true,
    configurable: true,
    value,
  });
}

...is now:

defineReplaceableLazyAttribute(globalThis, 'perf_hooks', ['performance']);

// ...

function defineReplaceableLazyAttribute(target, id, keys, writable = true) {
  let mod;
  for (let i = 0; i < keys.length; i++) {

    // ...

    ObjectDefineProperty(target, key, {
      __proto__: null,
      enumerable: true,
      configurable: true,
      get,
      set: writable ? set : undefined,
    });
  }
}

Notice that writable is now missing from the given options.

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

No branches or pull requests

4 participants