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

Core: Support selectively disabling Migrate patches #450

Merged
merged 1 commit into from Mar 14, 2022

Conversation

mgol
Copy link
Member

@mgol mgol commented Jan 4, 2022

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:

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 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 the warnings.md file.

There are a few special warnings printed directly via console.log without the migrateWarn* wrappers; they were left as-is; they should not have false positives and they're not doing anything except for console.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.

@mgol mgol requested review from dmethvin and timmywil January 4, 2022 15:25
@mgol mgol self-assigned this Jan 4, 2022
@mgol mgol added this to the 3.4.0 milestone Jan 4, 2022
Copy link
Member

@dmethvin dmethvin left a 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.

src/jquery/attributes.js Outdated Show resolved Hide resolved
src/jquery/attributes.js Outdated Show resolved Hide resolved
@mgol
Copy link
Member Author

mgol commented Jan 6, 2022

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.

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?

@dmethvin
Copy link
Member

dmethvin commented Jan 7, 2022

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!

@mgol
Copy link
Member Author

mgol commented Jan 14, 2022

@dmethvin Let's look a the toggleclass-bool example. The method is toggleClass but what should be the code for the warning? toggleClass-bool or toggleClassBool? The latter only partially matches the method due to the Bool suffix but OTH it's consistently camelCase. I think I slightly prefer toggleClass-bool but I can do both. I'd just like to know before I start updating the codes.

@dmethvin
Copy link
Member

Yeah the toggleClass-bool option sounds the best to me.

@timmywil
Copy link
Member

We talked about this in the meeting last week, but I still like this approach:

Step 1:

if we only warned when the patch is applied then we could just start with that one patch being disabled and having to enable it through jQuery.migrateEnablePatches when someone wants it; then we wouldn't need the UNSAFE_restoreLegacyHtmlPrefilter method

Step 2:

we could redefine jQuery.UNSAFE_restoreLegacyHtmlPrefilter() to just defer to the proper jQuery.migrateEnablePatches call, though

@mgol mgol changed the title WIP All: Support selectively disabling Migrate patches WIP Core: Support selectively disabling Migrate patches Jan 26, 2022
@mgol mgol changed the title WIP Core: Support selectively disabling Migrate patches Core: Support selectively disabling Migrate patches Jan 26, 2022
@mgol
Copy link
Member Author

mgol commented Jan 26, 2022

@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:

  1. src/disablePatches.js
  2. src/main.js
  3. src/jquery/manipulation.js
  4. test/migrate.js, there's non-trivial new logic there
  5. In test: ajax.js, attributes.js & css.js; all of them are passing on 3.x-git & git jQuery versions
  6. test/core.js from the beginning to jQuery.isWindow tests (inclusive) - they start at line 336 on main & line 426 in my PR

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.

@mgol mgol requested a review from dmethvin January 26, 2022 23:28
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
@mgol mgol force-pushed the migrate-disable-warnings branch from 1ea8d25 to 05a05b7 Compare March 1, 2022 23:08
@mgol mgol marked this pull request as ready for review March 1, 2022 23:13
@mgol
Copy link
Member Author

mgol commented Mar 1, 2022

@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 migratePatchAndWarnFunc or migratePatchFunc to do it as they cooperate with the mechanism I'm introducing here.

@mgol
Copy link
Member Author

mgol commented Mar 1, 2022

Note to self: update https://jquery.com/upgrade-guide/3.5/ once this is released to mention the new API.

Copy link
Member

@timmywil timmywil left a 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.

@mgol
Copy link
Member Author

mgol commented Mar 7, 2022

@dmethvin do you want to have a look as well? It's a bit different to my original implementation.

Copy link
Member

@dmethvin dmethvin left a 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!

@mgol mgol merged commit 5ee8f69 into jquery:main Mar 14, 2022
@mgol mgol deleted the migrate-disable-warnings branch March 14, 2022 16:31
mgol added a commit to mgol/jquery-migrate that referenced this pull request Jun 28, 2022
mgol added a commit to mgol/jquery-migrate that referenced this pull request Jun 28, 2022
The test setup broke because of jquerygh-450.

Tested on various setups locally.

Ref jquerygh-450
mgol added a commit that referenced this pull request Jul 6, 2022
The test setup broke because of gh-450.

Tested on various setups locally.

Closes gh-466
Ref gh-450
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support selectively disabling warnings/patches
3 participants