-
Notifications
You must be signed in to change notification settings - Fork 476
Core: Support selectively disabling Migrate patches #450
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
930203d
to
f964e09
Compare
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.
Happy new year!
Overall this seems fine, and not too bad as far as the size either. Being that naming things is the hardest problem in programming, I wonder if it would be better to use the actual camelCase names for things like deprecated methods. I know a few of them actually are groups so that breaks the pattern a bit. That's all nitpicks though, the general approach seems like it should work fine.
I guess it will make it easier. Maybe I can keep methods camelcased but if adding my own words, I can use hyphens? Or is such mixing bad? |
I'm not sure what's best to do for this. I like having the method name when it matches. Maybe just come up with some camelCase name for the group so the two aren't conflicting? Thanks for taking care of this! |
@dmethvin Let's look a the |
Yeah the |
We talked about this in the meeting last week, but I still like this approach: Step 1:
Step 2:
|
f964e09
to
1ea8d25
Compare
@dmethvin @timmywil This still needs a lot of work in tests and lots of modules don't work but I'd appreciate if you could review:
It'd be useful for me to get feedback about these changes before I spend more time on applying them to the rest of the tests in case major changes are needed. I tried to deduplicate this as much as I could but many of those attempts were hitting edge cases so it's not terribly DRY but I'm out of ideas on how to improve this. |
This adds three new APIs: * `jQuery.migrateDisablePatches` * `jQuery.migrateEnablePatches` * `jQuery.migrateIsPatchEnabled` allowing to selectively disable/re-enable patches in runtime. Source code is refactored to avoid manually overwriting jQuery methods in favor of using `migratePatchAndWarnFunc` or `migratePatchFunc` which cooperate with above methods. The `jQuery.UNSAFE_restoreLegacyHtmlPrefilter` API, introduced in Migrate 3.2.0, is now deprecated in favor of calling: ```js jQuery.migrateEnablePatches( "self-closed-tags" ); ``` The commit also reorganizes test code a bit, grouping modules testing patches to jQuery modules in a separate directory and extracting tests of Migrate APIs out of `core.js` to a separate `migrate.js` file. Helper files are now put in `test/data/` and unit tests in `test/unit/`; jQuery patch tests are in `test/unit/jquery/`. Fixes jquerygh-449
1ea8d25
to
05a05b7
Compare
@timmywil @dmethvin this is ready for a final review. The updated PR description provides more details. An important point is that after this lands we should no longer manually overwrite jQuery methods but we should always use |
Note to self: update https://jquery.com/upgrade-guide/3.5/ once this is released to mention the new API. |
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.
Looks great! Thanks for doing this.
@dmethvin do you want to have a look as well? It's a bit different to my original implementation. |
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.
Looks good. Thanks for doing this, I know it's pretty often requested!
The test setup broke because of jquerygh-450. Ref jquerygh-450
The test setup broke because of jquerygh-450. Tested on various setups locally. Ref jquerygh-450
This adds three new APIs:
jQuery.migrateDisablePatches
jQuery.migrateEnablePatches
jQuery.migrateIsPatchEnabled
allowing to selectively disable/re-enable patches in runtime. Source code is
refactored to avoid manually overwriting jQuery methods in favor of using
migratePatchAndWarnFunc
ormigratePatchFunc
which cooperate with abovemethods.
The
jQuery.UNSAFE_restoreLegacyHtmlPrefilter
API, introduced in Migrate 3.2.0,is now deprecated in favor of calling:
The commit also reorganizes test code a bit, grouping modules testing patches
to jQuery modules in a separate directory and extracting tests of Migrate APIs
out of
core.js
to a separatemigrate.js
file. Helper files are now put intest/data/
and unit tests intest/unit/
; jQuery patch tests are intest/unit/jquery/
.Fixes gh-449
Original description below
This introduces two new methods:jQuery.migrateDisablePatches
&jQuery.migrateEnablePatches
that allow to selectively (& dynamically) disable & enable jQuery Migrate patches by their IDs. Those IDs have been added to their respective entries in thewarnings.md
file.There are a few special warnings printed directly viaconsole.log
without themigrateWarn*
wrappers; they were left as-is; they should not have false positives and they're not doing anything except forconsole.log
calls.I originally planned to just silence warnings (see gh-449 for reasons why we need that) but that creates a danger: if a project fixes violations of a particular warning and silences it, the patch is still live so new violations can be introduced. Originally, I worried disabling patches may introduce weird cross-dependencies but the patches seem quite isolated so that seems doable.This is a work in progress. It needs tests for the new APIs as well as their effects on the patches. But I wanted to raise it to gather feedback about the proposed API and the implementation.