-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Remove Object.setPrototypeOf polyfill #125
Remove Object.setPrototypeOf polyfill #125
Conversation
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ 🚮 Removed packages: npm/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
We probably want to reflect this change in the |
152a761
to
8b51bf9
Compare
Hey, I know I have been slow to review things but I wonder if we could keep things open for a bit longer than a day? This was added originally to support browsers and we probably should have had a browser compatibility support listed somewhere and ideally tested, but it was a long time ago and I don't remember the discussions from back then. But to me this should have been considered a breaking change unless we were also going to consider 2.x having dropped browser supports. If we want that (which I think is a good idea) then we should also document this change as if it was part of what we dropped in 2.x. EDIT: for clarity, I dont mean we should back this out necessarily. Just that if we are saying we no longer support those old browsers we should document that as the intended behavior for 2.x entirely like we dropped node <18 even though the tests still passed in older nodes. EDIT 2: #14 This is interesting, we decided not to get the tests working and "advertise" browser support despite it being fully functional in the browsers at the time (and I am pretty sure it still is although I haven't used it that way in a few years). This is more of a meta question for how we want to manage support for browsers when the code can be used in them. Either way, I think before we release this we should decide on an approach and document it clearly so we don't break someone who was doing something a little unexpected but perfectly "valid" as a use case. |
Yep, l will work on small policy for this and other stuff related to PRs.
In this case I think that we are safe as I think that following with the proposal of this policy (expressjs/discussions#289) we should include tests for the things that we actually maintain. So, if the library was "unintended" working fine for the browsers probably we want to include tests for this otherwise "unintended" features might break at any time. Also this applies for any runtime that we don't specifically support, in this scenario I considered the browsers in this category. |
Yeah, I totally agree and removing it (and all the other unneeded polyfill type things) is a great thing. I was just being a stickler for semver 🤓. I think we can continue the browser support discussion in #128 and then revisit this once we make a decision there? |
This PR removes the
Object.setPrototypeOf
polyfill, which is no longer needed since this package now requires Node.js v18 as the minimum supported version. All existing tests should pass, as this change only impacts older Node versions no longer supported.