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

Upgrade prototype to version 1.7.3 #1497

Merged
merged 2 commits into from
Mar 27, 2021

Conversation

Sekiphp
Copy link
Contributor

@Sekiphp Sekiphp commented Mar 13, 2021

Description (*)

Upgrade prototype from version 1.7 to 1.7.3

http://prototypejs.org/

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added the JavaScript Relates to js/* label Mar 13, 2021
@kiatng
Copy link
Contributor

kiatng commented Mar 13, 2021

This update is long overdued. When I compare the official version here with this PR version, there are lots of discrepancies, all due to 2-spaces indent in the original versus the 4-spaces indent in the PR version. No biggie, but wouldn't it be better to use the official file as is? I think it'll help to verify the file easier.

@colinmollenhour
Copy link
Member

Just FYI I applied the same update to a different Magento-based project just now that has a fairly rigorous suite of functional tests using Cypress and it passed just fine.

@Sekiphp
Copy link
Contributor Author

Sekiphp commented Mar 13, 2021

@kiatng Fixed :-) I did not realise, that PHPStorm reformated file

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@Flyingmana Flyingmana left a comment

Choose a reason for hiding this comment

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

Compared old and new version with original source

@Flyingmana Flyingmana merged commit 2addcb1 into OpenMage:1.9.4.x Mar 27, 2021
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
2 runs  ±0  2 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 2addcb1. ± Comparison against base commit 57563fc.

@empiricompany
Copy link
Contributor

empiricompany commented Mar 25, 2022

this update of prototype from 1.7 to 1.7.3 break many things with 3rd party extension like proxyblue recaptcha, firecheckout and i also see many issues opened related to this change. so...i can ask why this update? there was some security / performance reason?

maybe we can mantain original version in the 19.4.15 release?

related issues 3rd party extensions:
firecheckout order review feature (unrecognize input:tel)

immagine

immagine

also in proxyblue recaptcha
ProxiBlue/reCaptcha#56

@kiatng
Copy link
Contributor

kiatng commented Mar 25, 2022

Prototype.js 1.7 is not compatible with vuejs, but 1.7.3 is. That is the reason I needed the upgrade.

@empiricompany
Copy link
Contributor

@kiatng but vuejs is not used in magento (it would be great if it were 😅), it's for more specific 3rd party extension not compatibile with original magento?

@kiatng
Copy link
Contributor

kiatng commented Mar 25, 2022

I have 1.7.3 in production for more than a year now, there's no issue with OM so far. For BC to 3rd party extensions, perhaps we can revert prototype.js in ver 19? And keep 1.7.3 in ver 20?

@empiricompany
Copy link
Contributor

my opinion is to revert to original prototype.js version because there is no real improvment or security issues and have a big impact in many areas, even because we had to be rewritten original library for issues with serialize function but I leave the choice to the maintainers @colinmollenhour @Flyingmana @LeeSaferite @drobinson @tmotyl @sreichel

@sreichel
Copy link
Contributor

If it breaks something, please revert - at least for 19.x branch.

@colinmollenhour
Copy link
Member

colinmollenhour commented Mar 29, 2022

@empiricompany What is the difference between input:tel and input[type=tel]? The latter works in all Prototype versions and seems to be the correct way anyway, but I'm probably missing something..

EDIT: no mention of input:tel here where I would expect it to be documented if it was part of the CSS standard.

I think supporting Vue, a widely used library on the same playing field as jQuery, is a strong reason to keep the latest version. Neither Vue or input:tel are used in the core Magento but that doesn't mean there aren't valid uses for them in private code or third-party extensions.

If using input[type=tel] is a valid workaround for this issue then why not just go with that as a fairly simple fix? That said, I'm not opposed to keeping the old version for v19 and using the latest for v20+. Are there any other known issues with upgrading to 1.7.3?

The most obvious resolution would be to keep the old in v19 and the new in v20.

@empiricompany
Copy link
Contributor

yes it is more correct to use input[type=tel] and the problem with firecheckout is solved (I will report it to them).
The update to prototype 1.7.3 also fixes the reported problem with ProxiBlue/reCaptcha#56 (sorry I got a little confused), although I found another problem in configuration with this commit https://github.com/OpenMage/magento-lts/pull/317/commits for which I will respond in the inherent pull request.
For me, updating the library for vuejs which, unlike jquery, is not used by third party extensions is not correct but it may be my thought too "conservative".
At this point I agree to leave the updated version 1.7.3 for v19 as well.
Sorry for the discussion but I just wanted to go deeper on this choice.

@kiatng
Copy link
Contributor

kiatng commented Mar 30, 2022

@empiricompany Ref prototypejs/prototype#337, v1.7.3 is not just for vuejs.

@empiricompany
Copy link
Contributor

@empiricompany Ref prototypejs/prototype#337, v1.7.3 is not just for vuejs.

thanks @kiatng this seems to me a more correct reason to update from my point of view

@ADDISON74
Copy link
Contributor

After this PR was merged, comments appeared in which issues related to functionality were reported. There were also some requests for revert. Because Prototype was updated in Magento 2 to the same version, I think we should analyze if they used the version from this PR or a modified one. Here is the PR merged in Magento 2 magento/magento2#33480

@ADDISON74 ADDISON74 self-assigned this Sep 15, 2022
@ADDISON74
Copy link
Contributor

I made a comparison between the two files and went through it carefully. I didn't find any differences except for some of the shape, added/deleted spaces or lines.

I recommend those who have issues with this PR to post them in the Issues section, also to follow the messages displayed in the browser console.

@ADDISON74 ADDISON74 removed their assignment Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScript Relates to js/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants