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

feat: bump jQuery to v3.6.0 #1397

Closed
wants to merge 3 commits into from
Closed

feat: bump jQuery to v3.6.0 #1397

wants to merge 3 commits into from

Conversation

trivikr
Copy link

@trivikr trivikr commented Aug 31, 2021

Description

Building on top of #1358

Testing

The generate API reference can be viewed at https://lsegal-yard-1397.netlify.app/

  • The jQuery version is "3.6.0", verified by running $.fn.jquery in Console.
  • The jQuery Migrate version is "3.3.2", verified by running $.migrateVersion in Console.
  • All toggle functionalities are working:
    • Table of Contents on landing page
      TOC.mov
    • Inheritance tree on Array page.
      inheritanceTree.mov
    • Source above footer on Array page.
      Source.mov
    • Defines on Module: YARD::Handlers::Ruby page.
      toggleDefines.mov

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@trivikr trivikr changed the title feat: bump jQuery to 3.6.0 feat: bump jQuery to v3.6.0 Aug 31, 2021
@trivikr
Copy link
Author

trivikr commented Aug 31, 2021

@lsegal This PR is ready for review.

@mullermp
Copy link
Contributor

:shipit:

@trivikr
Copy link
Author

trivikr commented Aug 31, 2021

Update: Added jQuery Migrate to not break downstream consumers with custom templates who may use deprecated APIs of jQuery.

The decision is based on prior feedback in:

Copy link

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me - Resolves the CVE and I believe this addresses issues with previous jquery upgrades.

@trivikr
Copy link
Author

trivikr commented Sep 1, 2021

I'll close this PR after rebase once #1399 is merged.
This way, we'll have PR to refer to if jQuery upgrade is prioritized in future.

@lsegal
Copy link
Owner

lsegal commented Sep 1, 2021

I'll close this PR after rebase once #1399 is merged.
This way, we'll have PR to refer to if jQuery upgrade is prioritized in future.

I'm not sure I understand this.

The intention is to backport toggle(), not remove it. #1399 is also not mergeable as is since it too is a breaking change.

The only way any of this can be merged is if the underlying jquery library is API compatible with the previous version distributed by YARD.

Custom templates (packaged as separate gems) depend on specific functions provided by YARD's templating system in the browser environment. Any changes to those functions would be equivalent to changes in Ruby methods. Replacing YARD's own internal API calls is secondary to this requirement.

TL;DR: toggle() must continue to be available to YARD's JavaScript templating system. Whether that's a custom implementation or a vendored jQuery backport doesn't matter, but the function has to exist.

@trivikr
Copy link
Author

trivikr commented Sep 1, 2021

The intention is to backport toggle(), not remove it. #1399 is also not mergeable as is since it too is a breaking change.

#1399 is not a breaking change, as it does not touch jQuery version vendored by YARD.
It just changes .toggle(function, function) calls to equivalent .click() calls in base templates.
Am I understanding it incorrectly?

TL;DR: toggle() must continue to be available to YARD's JavaScript templating system.

With #1399, the implementation of API .toggle(function, function) will continue to be available to YARD's JavaScript templating system through [email protected].
The PR is just replacing API calls to .toggle(function, function) in base templates, and not the implementation of the API.

@lsegal
Copy link
Owner

lsegal commented Sep 1, 2021

#1399 is not a breaking change, as it does not touch jQuery version vendored by YARD.
It just changes .toggle(function, function) calls to equivalent .click() calls in base templates.
Am I understanding it incorrectly?

It's not a breaking change per se, but it's also not a change that resolves any specific issue, and doesn't need to be merged.

The PR is just replacing API calls to .toggle(function, function) in base templates, and not the implementation of the API.

Sure, but I have the same comment for that issue as I did here: #1397 (comment) -- specifically, toggle() should nto be changed so we can validate that the underlying jQuery API has not changed. Changing the code to use click() would generate a false positive success case where your specific output test case looks valid but breaks for downstream consumers that still rely on toggle.

Leaving #1399 aside for the moment, this PR is still a breaking change and needs to be resolved before merging. #1399 should not be necessary to resolve the underlying jQuery upgrade and probably won't be merged because of the above comments.

@lsegal
Copy link
Owner

lsegal commented Sep 1, 2021

It is also worth noting here that the missing toggle() call in jquery.migrate.js indicates to me that it is explicitly not 100% API compatible with the current distributed version.

Do we know what other APIs have been changed/removed? A full list of changes would be useful to understand. Presumably every one of those functions would also need to be backported, otherwise we risk breaking downstream template authors that may depend on other functions outside of the obvious toggle() example.

@trivikr
Copy link
Author

trivikr commented Sep 1, 2021

It's not a breaking change per se, but it's also not a change that resolves any specific issue, and doesn't need to be merged.

The #1399 allows users of YARD to bump jquery to v3.6.0, as the new implementation .click() is forward compatible. It's explained in the Background and Description section.

Leaving #1399 aside for the moment, this PR is still a breaking change and needs to be resolved before merging.

I think there's a confusion. I don't plan to request merge for this PR.
I will just close it, as we will be unblocked once #1399 is merged and published.

@trivikr trivikr marked this pull request as draft September 1, 2021 21:45
@trivikr
Copy link
Author

trivikr commented Sep 1, 2021

Leaving #1399 aside for the moment, this PR is still a breaking change and needs to be resolved before merging.

I think there's a confusion. I don't plan to request merge for this PR.
I will just close it, as we will be unblocked once #1399 is merged and published.

Converted this PR to draft to avoid confusion.

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.

5 participants