Skip to content

Package doesn't work with --disable-proto=delete #4

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

Closed
dsherret opened this issue Nov 21, 2024 · 6 comments
Closed

Package doesn't work with --disable-proto=delete #4

dsherret opened this issue Nov 21, 2024 · 6 comments

Comments

@dsherret
Copy link

dsherret commented Nov 21, 2024

When I run the unit tests with NODE_OPTIONS=--disable-proto=delete I get the following error:

$ npm run tests-only

> [email protected] tests-only
> NODE_OPTIONS=--disable-proto=delete nyc tape 'test/**/*.js'

/mnt/v/typed-array-byte-offset/index.js:6
++cov_1i9ps04zcs.s[16];descriptor=gOPD(superProto,'byteOffset');}else{++cov_1i9ps04zcs.b[3][1];}// Opera 12.16 has a magic byteOffset data property on instances AND on Proto
                                  ^

TypeError: Cannot convert undefined or null to object
    at getOwnPropertyDescriptor (<anonymous>)
    at /mnt/v/typed-array-byte-offset/index.js:6:35
    at forEachArray (/mnt/v/typed-array-byte-offset/node_modules/for-each/index.js:12:17)
    at forEach (/mnt/v/typed-array-byte-offset/node_modules/for-each/index.js:54:9)
    at Object.<anonymous> (/mnt/v/typed-array-byte-offset/index.js:1:7537)
    at Module._compile (node:internal/modules/cjs/loader:1369:14)
    at Module.replacementCompile (/mnt/v/typed-array-byte-offset/node_modules/nyc/node_modules/append-transform/index.js:58:13)
    at module.exports (/mnt/v/typed-array-byte-offset/node_modules/nyc/node_modules/default-require-extensions/js.js:8:9)
    at Object.<anonymous> (/mnt/v/typed-array-byte-offset/node_modules/nyc/node_modules/append-transform/index.js:62:4)
    at Module.load (node:internal/modules/cjs/loader:1206:32)

Node.js v20.12.2

I think this is because the has-proto only checks if __proto__ is allowed on object creation and not after the fact?

@ljharb
Copy link
Member

ljharb commented Nov 21, 2024

You're right; the semantics of has-proto are definitely "can be set in an object literal" and not "the accessor exists".

I'm not sure I care to support this particular node flag - it's just not reasonable to enable it with how many parts of the ecosystem it breaks. It wouldn't be hard to add, but it'd add an extra dependency on reflect.getprototypeof.

@dsherret
Copy link
Author

dsherret commented Nov 21, 2024

It wouldn't be hard to add, but it'd add an extra dependency

Would you be willing to support this scenario for where the Reflect global exists so a dependency doesn't need to be added (if __proto__ isn't supported then likely Reflect will exist)? Also, it seems the dependency on has-proto could be removed since it's not checking the right thing?

@ljharb
Copy link
Member

ljharb commented Nov 21, 2024

Indeed, it'd remove the has-proto dep. My robustness constraints don't allow me to solely rely on Reflect, though - I'll probably end up adding the dep.

@ljharb ljharb closed this as completed in 9dde370 Nov 21, 2024
@dsherret
Copy link
Author

Thanks! Not relying on __proto__ slowly moves the ecosystem in a better place.

@ljharb
Copy link
Member

ljharb commented Nov 21, 2024

I think the accessor is fine as-is, personally, and removing it isn't "better", but correctness does matter, and it was clearly incorrect. (also, an environment without it isn't actually an environment built on web standards, since the web requires it to always be present)

v1.0.3 will be published later today.

ljharb added a commit that referenced this issue Nov 22, 2024
@ljharb
Copy link
Member

ljharb commented Nov 22, 2024

v1.0.3 is released. I'll also add some regression tests.

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

2 participants