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

upgrade ES target to es2022 #273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

keithamus
Copy link
Member

This upgrades the output to use ES2022. This includes native private fields syntax. We see almost zero devices on github.com that do not support private fields. And we've been considered private fields part of our baseline support for a while now.

This means users on macos Mojave and below, using Safari, may have a further degraded experience than they already have. They can improve their experience by switching to another browser such as Chrome or Firefox, but Apple does not release Safari 15 for macos Mojave or below, as Apple has not supported macOS Mojave since Oct 2021 - when it was EOLd.

This also means users on ios/ipados 14 may see a further degraded experience. ios/ipados 14 was EOL in October 2021, and Apple no longer provides security updates for these devices. Some of these devices can upgrade to ios/ipados 15. Some devices, like the iPad Mini 3 (discontinued October 2014), 4th generation iPad (discontinued October 2014), or the iPhone 6 (discontinued September 2016) will be unable to upgrade to iOS 15, and so visiting a page with <relative-time> will cause JS to SyntaxError.

@keithamus keithamus requested a review from a team as a code owner February 21, 2024 12:23
@silverwind
Copy link

silverwind commented Mar 13, 2024

FWIW, I avoid private fields in all my code because of this. Not sure if it's really relevant to this element or whether consumers would install Proxies on it.

@keithamus
Copy link
Member Author

I'm not sure I share those opinions; if you're extending a class it's fairly common to have to override parts of the class to deal with things like private state or copying (see the whole @@species debacle). The alternatives to private fields are: make everything public (but then lose the ability to observe changes or have two public fields), or use a WeakMap, and then the loud errors about private field access turn into silent ones where properties using a WeakMap can return undefined instead.

@silverwind
Copy link

make everything public

I prefer all-public fields because they enable advanced use cases that the original author has not considered. For example, there are numerous cases in Node.js API where one needs to access underscore props. With private fields, you restrict the consumer too much imho.

@keithamus
Copy link
Member Author

Many of the private fields in this codebase (and our other custom element codebases) are public, but they need to be observed, for example #onRelativeTimeUpdated has a get/set but it has (warranted) side-effects which can only happen if we store the state under a different name. We could publicise the field name but that means coming up with some alternative name, and runs the risk of exposing undesirable information which can cause memory leaks - for example setting onRelativeTimeUpdated removes the old event listener, but if one were to assign to the #onRelativeTimeUpdated and not remove the old event listener, it would continue to fire and could cause further errors. We could make this field a WeakMap instead, but as I say, we then face the same issue with regard to subclassing, so I feel like this is a well justified use case where the alternatives have greater trade-offs.

@silverwind
Copy link

silverwind commented Mar 13, 2024

Okay. I'm not suggesting changing these private fields now, but for new code, I would definitely consider using them and if there's no strong reason, I wouldn't, so that I don't break Proxy usage.

On topic: Another browser often forgotten is Pale Moon. It looks to support private class fields since their 2023-05-16 release, so I think it'll likely be fine with this change.

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.

3 participants