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

Fix for .data() method to read scientific notation as string as is. #399

Closed
wants to merge 8 commits into from

Conversation

jasongodev
Copy link

Problem

<div class="data-scientific" data-unsigned="1234e5" data-epositive="1234e+5" data-enegative="1234e-5"></div>

// This is true
jQuery('.data-scientific').data('unsigned') === '1234e5';

// This is false, because cash will return 123400000 due to JSON.parse('1234e5') parses it to a number.
cash('.data-scientific').data('unsigned') === '1234e5';

Solution

  • test for scientific notation using regex, if true then return value and do not JSON.parse'd it

@fabiospampinato
Copy link
Owner

  1. Can you delete all the dist and unnecessary changes from this PR?
  2. Looking at the code shouldn't this be working already? Like JSON.parse ( '1e99' ) is parsed correctly for me, is the string not reaching JSON.parse for some reason?

@jasongodev
Copy link
Author

jasongodev commented Apr 21, 2022

Hi Fabio, JSON.parse will convert the '1e99' into a number. What we want for .data() is to return '1e99' as '1e99' and not as 1000000000. So before we pass the string to JSON.parse, we need to check if it has a scientific notation format. This is consistent with jQuery, Zepto, and native Element.dataset.get() routine.

I will revert the dist and other edits from this pull request.

Thanks!

@fabiospampinato
Copy link
Owner

Thank you but our data function is explicitly not fully aligned with jQuery, as jQuery's implementation doesn't work with stuff like React, which some people, me included, use alongside more direct DOM manipulation libraries.

Moreover if there's a data-foo="123"attribute somewhere jQuery's data function will read that as a 123 number, so not reading things like 1e10 as a number too seems inconsistent to me, this should probably be something jQuery should change, not the other way around.

@jasongodev
Copy link
Author

I respect your decision on this.

Regarding jQuery and Element.dataset, they probably based it on HTML Spec from WHATWG, where values of dataset properties are returned as DOMString.

https://html.spec.whatwg.org/multipage/dom.html#domstringmap
https://webidl.spec.whatwg.org/#idl-DOMString

Still, cash-dom's .data() method might be a more practical implementation in real world case than it is to follow the specs strictly.

Thank you for your review and have a nice day!

@fabiospampinato
Copy link
Owner

fabiospampinato commented Apr 21, 2022

You might consider using the attr method if what you want is always a string. Potentially you could also override Cash's data method. Unfortunately the best way to write such function isn't obvious, there are pros and cons of both approaches.

Sorry for the inconvenience.

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.

None yet

2 participants