Skip to content

Conversation

fulv
Copy link
Contributor

@fulv fulv commented Jul 24, 2018

No description provided.

@fulv
Copy link
Contributor Author

fulv commented Jul 24, 2018

For the record:

WARN: 'JQMIGRATE: jQuery.fn.load() is deprecated'

This is caused by the jquery.anythingslider plugin, which has jQuery pinned at ">=1.4.2", so it's probably expected to work even with old versions of jQuery.

@fulv
Copy link
Contributor Author

fulv commented Jul 24, 2018

The warning

WARN: 'JQMIGRATE: $(html) HTML text after last tag is ignored'

was caused by this type of code, which was repeated in multiple places:

var $el = $('<div class="pat-stacks"></div');

Note the missing final angle bracket. I'm not sure if this was intentional, or simply a typo that proliferated due to copy & paste.

Ref: https://github.com/jquery/jquery-migrate/blob/1.x-stable/warnings.md#jqmigrate-html-text-after-last-tag-is-ignored

@fulv
Copy link
Contributor Author

fulv commented Jul 24, 2018

Now we only have the following warnings left:

WARN: 'JQMIGRATE: jQuery.browser is deprecated'
WARN: 'JQMIGRATE: jQuery.browser is deprecated'
WARN: 'JQMIGRATE: jQuery.fn.load() is deprecated'
WARN: 'JQMIGRATE: jQuery.fn.load() is deprecated'
WARN: 'JQMIGRATE: Can't change the 'type' of an input or button in IE 6/7/8'
WARN: 'JQMIGRATE: Can't change the 'type' of an input or button in IE 6/7/8'
WARN: 'JQMIGRATE: Can't change the 'type' of an input or button in IE 6/7/8'
WARN: 'JQMIGRATE: Can't change the 'type' of an input or button in IE 6/7/8'

The first one is probably going to be tricky.
This can probably be remedied by judicious use of http://api.jquery.com/jQuery.support/, or modernizr.

fn.load(), as mentioned in an earlier comment, is caused by jquery.anythingslider, which is only used by carousel-legacy. Not sure if we should drop this at some point, or build it with our own fork of anythingslider.

The JQMIGRATE: Can't change the 'type' of an input or button in IE 6/7/8 can safely be ignored, in my opinion.

Please evaluate whether to merge this now, to provide a baseline for further upgrade to jQuery 3. Either way, I'm planning on building on top of this branch for further migration work.

@fulv
Copy link
Contributor Author

fulv commented Aug 6, 2018

I'll leave the tests to fail until I'm done with all the fixes, instead of adding jquery-migrate.

@pilz
Copy link
Member

pilz commented Aug 6, 2018

Really nice progress.
Note that we don't really support IE6/7/8/9 and even 10 is out of scope as it is not even supported by MS anymore. I believe to remember that the jquery.browser stuff was to work around some old IE bugs. If that is correct, I would be fine by removing that compatibility code instead of doing a costly replacement of jq.browser.

@fulv
Copy link
Contributor Author

fulv commented Aug 7, 2018

@pilz good to know, I will keep that in mind.

@fulv
Copy link
Contributor Author

fulv commented Aug 20, 2018

I figured out the issue:

WARN: 'JQMIGRATE: jQuery.fn.offset() requires an element connected to a document'

It comes from select2: select2/select2#5173

I have not tried different versions of select2 to see whether it goes away, because the above issue is still open, so I guess there is nothing we can do.
From what I can see, there is no other code causing this warning other than select.

FTR, this is where the warning gets triggered: https://github.com/select2/select2/blob/stable/3.5/select2.js#L1386

@fulv
Copy link
Contributor Author

fulv commented Aug 21, 2018

And this concludes the migration. The remaining warnings are all due to external dependencies, i.e. select2 and anythingslider:

WARN: 'JQMIGRATE: jQuery.fn.bind() is deprecated'                                                                                                                                    
WARN: 'JQMIGRATE: jQuery.fn.bind() is deprecated'                                                                                                                                    
WARN: 'JQMIGRATE: jQuery.fn.unbind() is deprecated'                                                                                                                                  
WARN: 'JQMIGRATE: jQuery.fn.unbind() is deprecated'                                                                                                                                  
WARN: 'JQMIGRATE: jQuery.fn.load() is deprecated'                                                                                                                                    
WARN: 'JQMIGRATE: jQuery.fn.load() is deprecated'                                                                                                                                    
WARN: 'JQMIGRATE: jQuery.fn.offset() requires an element connected to a document'                                                                                                    
WARN: 'JQMIGRATE: jQuery.fn.offset() requires an element connected to a document'                                                                                                   

I verified this by hacking select2 and anythingslider in node_modules, and by doing so I was able to get a totally clean test run.

Next steps:

  • Update the dependencies which are mentioned by npm and github as outdated or containing vulnerabilities by uppinning
  • Checking tests again
  • Creating a test bundle and run our integration tests with the new jquery bundle
  • Do manual tests with the new bundle.

@pilz
Copy link
Member

pilz commented Aug 21, 2018

Excellent. Can you try to merge current master in? I think it already works with a modern select2, that might fix it. Perhaps there is also a newer version of anythingslider?

https://github.com/patternslib/Patterns/network/dependencies shows 11 vulnerabilities, the next step should be to upgrade them to latest version and check again.

@fulv
Copy link
Contributor Author

fulv commented Aug 25, 2018

@pilz where do you see those 11 vulnerabilities? When I go to https://github.com/patternslib/Patterns/network/dependencies, I can't find anything related to vulnerabilities.

@pilz pilz merged commit b22caed into master Apr 29, 2019
@fulv
Copy link
Contributor Author

fulv commented Apr 30, 2019

whoa!

🎆 ⚡️ :feelsgood: 😌 😆 💥 👊 👏 👯 💌 💪 👍 😂 😁 😤 🆗 👌 🙆‍♂ 🙆 🤛 👊 ✊ 🤜 🤘 😹 :octocat: 🎉 💣 🎂 🎁 💝 🎈 👑 💎 💴 💶 💷 💵 💳 💰 🎊 🎄 🎅 🤶 🎏 🏁 🌠 🏮 ㊗️ ✳️ 🔝 💟 💲

@fulv fulv changed the title DO NOT MERGE: jquery-migrate WIP jquery-migrate Apr 30, 2019
@pilz
Copy link
Member

pilz commented Apr 30, 2019

nice, eh? I do see some random failures on master now which don't occur locally, and may be timing issues. They are always on pat-scroll but the failing test is also changing randomly. I don't consider that a problem, but annoying.

@fulv
Copy link
Contributor Author

fulv commented Apr 30, 2019 via email

@fulv fulv removed the in progress label May 4, 2019
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.

3 participants