mozilla-jetpack / jpm

Jetpack Manager for Node.js
https://www.npmjs.org/package/jpm
Mozilla Public License 2.0
164 stars 75 forks source link

Odd Array/String coercion in lib/rdf.js #584

Closed pdehaan closed 8 years ago

pdehaan commented 8 years ago

Ref: lib/rdf.js:250,

// TODO: Figure out why this fails w/ `===`
if (Object.keys(descriptionData) == "em:targetApplication") { // eslint-disable-line eqeqeq

This feels very dirty to me. Object.keys() returns an object Array, but we're trying to compare it to a string (which works with == but not with strict ===).

const foo = ['bin'];
const bar = 'bin';

console.log(foo == bar); // true
console.log(foo === bar); // false

I had to add an // eslint-disable-line eqeqeq and live with my never ending abyss of ESLint shame.

kumar303 commented 8 years ago

Huh, looks like this was a bug. Who knows. The jpm legacy is rife with obscure javascript tricks like return ~DEBUG_VALUES.indexOf(option) 🚑

I think this line can definitely be changed. However, I have always been a little hesitant to change little things like this because the test coverage isn't great. Don't let that stop you though! We have a passionate user base who are great about filing bugs after new releases.

kumar303 commented 8 years ago

... because the test coverage isn't great

And the functional test suite is especially cumbersome to deal with

pdehaan commented 8 years ago

Off the top of your head, do you know what the fix is?

if (Object.keys(descriptionData).indexOf("em:targetApplication") !== -1) {...}

I'm kind of amazed that the code ever worked (and an Array could be coerced into a string, so I'm mentally trying to wrap my head around what the coercion logic is.

freaktechnik commented 8 years ago

I think it works only for one element arrays, so you should be able to just compare the first element, or even better use the in operator or hasOwnProperty directly on descriptionData.

pdehaan commented 8 years ago

Submitted #588

Spoiler alert:

if (descriptionData.hasOwnProperty("em:targetApplication")) {...}