-
Notifications
You must be signed in to change notification settings - Fork 16.2k
refactor!: remove unused ImageView
browser module
#46607
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
Conversation
BREAKING CHANGE In case any users came across this feature somehow and are using undocumented API, I'm marking this as a breaking change to ensure it gets noted in the release notes.
Adding context: The purpose of the API was to enable splash screens that will be swapped for Usage example: https://gist.github.com/itsananderson/fc8b735fdf19714eee1d39e2c427b7e3 Relevant governance PR: electron/governance#254 Feature request for splash screens: #39728 There are also more comments in this Slack thread. I have no strong feelings about what to do with it. |
If I may, I'd like to delay merging this for maybe 3 months. I may have a potential use case for this that I'd like to try out 🙏 I certainly see the appeal of removing code which isn't utilized though so. |
I've found people in the wild who are implementing splash screens through (Not optimal in my opinion.) |
My two cents is if the code/maintenance burden is considered small enough (which I think could be argued in this case) that we should keep it and document it. I think it fills a valid use case and some apps might adopt it if it were documented. |
@ckerr output from last API meeting is we should document this as experimental, not remove it - would you rather we do that in this PR or close this and follow up with a new one? |
@codebytere I'll update this PR tomorrow to mark it as experimental 👍 |
Actually, might be better to do it in a new PR. It doesn't make much sense for a branch named 'refactor/remove-unused-ImageView' for this task. I'll close + delete this branch, and a stakeholder in @electron/wg-api can create a new branch to add documentation. |
Here's the follow-up issue for documenting this: #46601 |
Oh, I see we already have a new PR: #46760 Thanks, @itsananderson! |
Description of Change
BREAKING CHANGE
We should either remove or document the
ImageView
browser module. It's been around for several years now and we pay maintenance costs on it, e.g. in the Upgrades WG, but it's never been public API.My preference would be to remove 😈 , since you don't have to do maintenance on code that doesn't exist. But maybe people would use it if we documented it? When I mentioned in #ask-anything that this was unused, @nikwen opened a docs ticket suggesting that. 👍
So this sounds like a decision for @electron/api-wg
(Technically it's not a breaking change to remove undocumented API, but I think we do have prior history on listing those changes as breaking Just In Case someone came across the feature on their own and is using it anyway, so that's why I'm listing this as a breaking change in this PR)
Checklist
npm test
passesRelease Notes
Notes: Removed undocumented
ImageView
browser module.