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

js-yaml introduces changes in behaviour #21

Open
confuser opened this issue Apr 4, 2016 · 5 comments
Open

js-yaml introduces changes in behaviour #21

confuser opened this issue Apr 4, 2016 · 5 comments

Comments

@confuser
Copy link

confuser commented Apr 4, 2016

Authentication in one of our dev APIs was failing.

A client secret is defined as an integer within a config yaml file. Since the change over to js-yaml within this library, this means it's now being interpreted differently for some reason, and being rounded up.

Forcing it to a string by wrapping it within quotes does solve this problem but it's a major change in previous behaviour; which was quite unexpected and has caused a significant amount of time tracking this down on our side.

Example:

anAPI:
  clientSecret: 8421999999999995999
a127.init(function (config) {
    console.log(config.anAPI.clientSecret)
})

clientSecret becomes 8421999999999996000

Using 0.12.1 of a127-magic. Reverting to 0.11.2 this issue does not occur.

@theganyo
Copy link
Contributor

theganyo commented Apr 4, 2016

I'm sorry you ran into trouble and had to expend effort tracking it down.

I've verified that this issue is, in fact, not precisely a problem with the yaml parser as it is actually has more to do with the interpretation of "integer" string by javascript. Perhaps the old yaml library interpreted the value as a string for some reason, but you can see that the Javascript interpreter itself (at least V8) does the rounding of the value if it's interpreted as an integer (which seems to be a correct interpretation according to the YAML spec):

node -e "console.log(8421999999999995999)"
8421999999999996000

Given this, I believe the resolution to simply quote the value to ensure it's interpreted as a string is not an unreasonable one.

That said, I've opened an issue with the js-yaml project since this is clearly unexpected behavior and other parsers don't have the same issue.

@confuser
Copy link
Author

confuser commented Apr 4, 2016

It's interpreted by default as a string within yamljs. Thanks for opening an issue with js-yaml.

@theganyo
Copy link
Contributor

theganyo commented Apr 4, 2016

You bet. Although at this point it doesn't look as if they're especially interested in addressing it.

@confuser
Copy link
Author

confuser commented Apr 7, 2016

Indeed, doesn't look like it's going anywhere.

What's your preferred resolution? It's MIT so happy to fork on our side (Burberry) and add a warning notice when this occurs. Unless it's preferable for Apigee to take ownership over that. But then that'll add additional maintenance, so perhaps simply note this as a caveat on the readme for magic?

@theganyo
Copy link
Contributor

theganyo commented Apr 8, 2016

If you're willing to submit a simple PR with the warning, perhaps they'd accept it? Otherwise, I'm thinking that we'd just document the issue...

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

No branches or pull requests

2 participants