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

Remove unneeded polyfills #2683

Merged
merged 5 commits into from Nov 27, 2021
Merged

Remove unneeded polyfills #2683

merged 5 commits into from Nov 27, 2021

Conversation

NicolasCARPi
Copy link
Collaborator

Start a little code cleanup by removing code that doesn't need to be here in 2021.

See https://caniuse.com/?search=classlist
See https://caniuse.com/?search=startswith
See https://caniuse.com/?search=Event

@NicolasCARPi NicolasCARPi merged commit 662a3fd into dev Nov 27, 2021
@NicolasCARPi NicolasCARPi deleted the polyfill branch November 27, 2021 02:27
@caseyjhol
Copy link
Member

We'll have to make sure this is only available in the v2.0.0 release, as it breaks IE11 support.

@NicolasCARPi
Copy link
Collaborator Author

@caseyjhol I'm not sure if we need to do that. Honestly we don't have the resources to support deprecated browsers. We need to move forward and unless you have people specifically paying you to keep IE 11 support, there is no reason to keep such code around. We of course need to specify it in the changelog, but I really don't think this change should be held back until an hypothetical v2.

Have you used windows recently? Microsoft is pushing for Edge VERY hard into the throat of users. Maybe some companies rely on this lib working with IE11 or less, but is it our problem? No. They won't update it, that's it. Not our problem.

You see, over the years the codebase became HUGE. This PR removes MANY lines that are now completely useless for 99.99% of users. The bootstrap-select.js file is still ~3400 lines, with comments like "for backward compatibility", "fix for IE", "fix for firefox 25", etc... All of these were added years ago and are no longer relevant. They need to go away.

Another thing that I've seen many times in open source projects, is wanting to make one big release (v2) or one big rewrite. At the end of the day, it's not done because it implies a lot of work. So IMHO it's much better to do incremental changes. And major version change should only be if the API of the lib changes drastically, not because we stop support for old deprecated browsers.

@caseyjhol
Copy link
Member

I agree we don't need to support outdated browsers forever, but there are still quite a few businesses that are stuck on IE11 (including many customers who use SnapAppointments). I'd like to try to stick to SEMVER somewhat, and dropping support for IE11 warrants a new major release IMO. v2 doesn't have to be some major rewrite, but I'd also like to implement more changes than removal of a few lines of code. The proper way to go about this would be to move the polyfills to a separate file (that gets combined during the build process), and mark IE11 support as deprecated. The file could then get removed in the next major release. At the end of the day, keeping those polyfills isn't adding a ton of (or any?) extra lift to the development process, and it only adds a few minified kilobytes to support thousands of extra users.

@NicolasCARPi
Copy link
Collaborator Author

I agree that we should bump to 2.0.0, but without trying to implement the long list of things you planned on #2228.

The proper way to go about this would be to move the polyfills to a separate file

I'm not sure. In fact polyfills are not anymore the problem of upstream libs, because the build process can include babel that will take care of that much better than us in the lib. Of course that means we shift responsability to another place. But I believe it's a good thing. Want IE support? Use Babel. End of story, we as open source developers don't need to bother with antique browsers. That's my point of view (which might be seen as radical, but life is too short to bother with IE :p).

So maybe we're talking about a non problem here, because all IE users use babel anyway to bundle their code and it's possible that this code was never actually useful for them (you see what I mean?). It's an interesting discussion.

@caseyjhol
Copy link
Member

Yeah moving the polyfills to a separate file sounds like more work than it's worth. I say we keep them as is until v2.0.0. For v2.0.0 we should identify some other key/easy breaking changes we can implement at the same time (I think we can do some of the things from that list), but it doesn't need to be anything drastic.

I'd also like to maybe implement plugin support (similar to Selectize), so we could keep the base code lean but still add support for things like tags/combobox down the road. Thoughts?

NicolasCARPi added a commit that referenced this pull request Nov 28, 2021
@NicolasCARPi
Copy link
Collaborator Author

I say we keep them as is until v2.0.0

I'll revert this PR then.

I'd also like to maybe implement plugin support, so we could keep the base code lean but still add support for things like tags/combobox down the road. Thoughts?

I've never been a fan of plugins, as it means you open your stuff to random people that might write very bad plugins and it can damage the image of the lib. Unless you whitelist plugins but then it adds more maintenance. It also doesn't make the code better because you then have to place hooks everywhere and check more stuff "because some plugins might do this"... My 2 cents.

IMHO the most important thing right now is to focus on releasing a new version, as the latest stable is pretty old now. Then improve the codebase by removing cruft and refactoring, splitting, rearranging code.

Splitting the huge file into multiple ones is something that I think would be good, too. My view is that JS libs need to embrace modern tooling, so for me that means using typescript and es6 import/export. We can start by at least using const/let instead of var, as a first step.

Another important point is adding tests. Automated ones. I don't think unit tests make a lot of sense for this project, but end to end test sure do. I have some experience with Cypress. We already have an html page to test stuff but it's not automated. I'll probably make a PR to add this. I feel much better hacking a codebase when the stuff is tested and I can be half-confident that nothing broke badly ;)

caseyjhol pushed a commit that referenced this pull request Nov 30, 2021
@caseyjhol
Copy link
Member

Yes - some automated testing would be amazing! Been wanting to add it for years.

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.

None yet

2 participants