-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
jquery-patch.js: Misleading label and description ("Support version 1.8.x and newer of jQuery core") #2200
Comments
I think the name of the file is fine since it does patch jQuery Core. Updating the metadata sounds good to me. I think I'd use uppercase "Core". PRs welcome! |
Whether the file patches jQuery Core is arguable, but regardless of whether the name is correct or not, it is not good. The file could be named "small-file-which-contains-functions.js", which would be 100% correct, but that would certainly not be a good name, as it describes implementation rather than role. A good name should focus on the component's objective rather than on its internals. I agree that "Core" should be capitalized. |
It’s not just internals, though. Loading jQuery UI with jQuery 1.8 makes methods |
I’m not married to the name, BTW, but this is churn that we even cannot really introduce in a patch release, it’d have to wait for 1.14 if we release one at all. I’m not too keen planning such things, especially that jQuery UI is in maintenance mode and there are many more changes that would be good to do but we will likely never do them. cc @fnagel for your opinion |
@mgol I'm fine with changing label and description in the download builder as @Chealer has a point here, the label is somehow confusing and could be improved.
@Chealer Why? It patches jQuery, not jQuery UI. |
@fnagel Oh sorry, I meant to emphasize patches, not Core. The word "patch" has been largely extrapolated since, but originally, it described tape patches applied over previous tape, therefore replacing previous code. jquery-patch.js does not replace jQuery, it merely extends it. |
Right. What I meant by internals was not that it was invisible outside, but that it's an implementation detail. That these are visible not just to jQuery UI is both accidental and undesirable. |
I agree but at this point we'd prefer to avoid introducing breaking changes by removing this module. |
I am fine with that @mgol , I was not saying this should be changed. To clarify what I meant to say: |
The current plan for 1.14 is to keep the file for backwards compatibility but to not depend on it in code. This means the only purpose for it will actually be patching jQuery and not making jQuery UI compatible with older jQuery. |
The jquery-patch.js component increases backwards compatibility by allowing current jQuery UI versions to be used with jQuery core versions anterior to 3.5 (until 1.8). jQuery core 3.5+ would otherwise be required, as explained in the Newer Core features section of the jQuery UI 1.13 Upgrade Guide:
Unfortunately, that is pretty far from what one understands when using the Download Builder, which calls it "jQuery 1.8+ Support" and describes its role as "Support version 1.8.x and newer of jQuery core".
This name and this description come from the label and description defined in the file's header. Even though the previous versions were poor/outdated, they became misleading and fragile when support for core 1.7 was dropped.
The new texts make it backwards, implying that jquery-patch provides compatibility with new versions, rather than with old versions. Moreover, it makes the information time-sensitive, as while they may not be quite wrong at this time, there is no guarantee that jQuery UI 1.13 will be compatible with jQuery core 4.
I suggest the following:
By the way, this file's role would be much clearer if it was named "legacy-jquery-core.js".
The text was updated successfully, but these errors were encountered: