Add fix for $.fn.data calls #437
Conversation
When calling `$(el).data()`, the received object contains camel-cased keys, this fixes issue jquery#436
| var args, result; | ||
| if ( arguments.length === 0 && typeof Proxy !== "undefined" ) { | ||
| result = oldFnData.call( this ); | ||
| // eslint-disable-next-line no-undef |
There was a problem hiding this comment.
Why is it needed when all variables are used?
There was a problem hiding this comment.
I got an eslint error that Proxy is not defined, could it be only for me?
There was a problem hiding this comment.
Ah, this. Instead of silencing the whole rule, please add another override entry to src/.eslintrc.json. There's already one for css.js, you can copy it and just include Proxy as we don't use Reflect here.
src/jquery/data.js
Outdated
| if ( arguments.length > 0 && typeof key === "string" && key !== camelCase( key ) ) { | ||
| migrateWarn( | ||
| "jQuery.data() always sets/gets camelCased names: " + key | ||
| ); | ||
| args = | ||
| arguments.length > 1 ? | ||
| [ camelCase( key ), value ] : | ||
| [ camelCase( key ) ]; | ||
| return oldFnData.apply( this, args ); | ||
| } |
There was a problem hiding this comment.
$.fn.data('key-name') already normalizes to camelCase, why is this conversion needed?
|
Hey @liady, are you interested in finishing this PR? |
|
@liady are you willing to finish the PR? |
This comment has been minimized.
This comment has been minimized.
|
@mgol this is an important fix so I do want to implement the changes to this PR, however I cannot attend to it in the next 10 days. |
| @@ -38,3 +39,42 @@ jQuery.data = function( elem, name, value ) { | |||
|
|
|||
| return oldData.apply( this, arguments ); | |||
| }; | |||
|
|
|||
| jQuery.fn.extend( { | |||
| data: function( key, value ) { | |||
There was a problem hiding this comment.
Please use tabs for indentation, not spaces. This applies to most of the added lines.
| @@ -1,7 +1,8 @@ | |||
| import { migrateWarn } from "../main.js"; | |||
| import { camelCase } from "../utils.js"; | |||
|
|
|||
| var oldData = jQuery.data; | |||
| var oldData = jQuery.data, | |||
| oldFnData = jQuery.fn.data; | |||
There was a problem hiding this comment.
Please use tabs for indentation, e.g. here:
| oldFnData = jQuery.fn.data; | |
| oldFnData = jQuery.fn.data; |
| var args, result; | ||
| if ( arguments.length === 0 && typeof Proxy !== "undefined" ) { | ||
| result = oldFnData.call( this ); | ||
| if(!result) { |
There was a problem hiding this comment.
if needs to be followed by the space. Please have a look at the GitHub Actions output, it shows you all the various ESLint errors of this PR.
| var args, result; | ||
| if ( arguments.length === 0 && typeof Proxy !== "undefined" ) { | ||
| result = oldFnData.call( this ); | ||
| // eslint-disable-next-line no-undef |
There was a problem hiding this comment.
Ah, this. Instead of silencing the whole rule, please add another override entry to src/.eslintrc.json. There's already one for css.js, you can copy it and just include Proxy as we don't use Reflect here.
|
@liady Hey, could you provide a status update? |
When calling
$(el).data(), the received object contains camel-cased keys, this fixes issue #436