-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Throw an error if config.storeName
isn't a String
#948
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a neat idea, but it looks like your code is failing the lint checks (and from the looks of it will fail the tests).
Would you be willing to write a test case for this new code path? If you could do that and fix the other issues I mentioned, I think we could add this, thanks! 😄
src/localforage.js
Outdated
return this._config[options]; | ||
} else { | ||
return this._config; | ||
} catch (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't catch any error here; instead this should throw as it did previously.
src/localforage.js
Outdated
if (i === 'storeName' && typeof options[i] !== 'string') { | ||
options[i] = options[i].replace(/\W/g, '_'); | ||
} else { | ||
return new Error('storeName must be a string'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a helpful touch, and I guess it isn't a breaking change since .replace
on anything that isn't a String
would fail anyway (since it isn't a function).
src/localforage.js
Outdated
if (i === 'version' && typeof options[i] !== 'number') { | ||
return new Error('Database version must be a number.'); | ||
for (let i in options) { | ||
if (i === 'storeName' && typeof options[i] !== 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this code it seems like this if
check will fail (calling the else
block below) for any option key that isn't storeName
. You'll want to nest the check for (typeof options[i] !== 'string')
inside this if
statement or this will break other things.
src/localforage.js
Outdated
for (let i in options) { | ||
if (i === 'storeName') { | ||
options[i] = options[i].replace(/\W/g, '_'); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this try/catch
block here is helpful to add, as it will prevent unexpected errors from throwing.
src/localforage.js
Outdated
@@ -314,7 +322,7 @@ class LocalForage { | |||
} | |||
|
|||
function initDriver(supportedDrivers) { | |||
return function() { | |||
return function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why these code changes were made, but they're failing the linter; could you please revert these style/whitespace changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got this due to correcting the indentation by some extension. I'll revert it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think test cases in the test folder needed to be more organized in categories for manging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with the changes and also added a test case in test/test.config.js regarding this issue fixing test. Can you check it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get any update from you, sir.
config.storeName
isn't a String #947
config.storeName
isn't a String #947config.storeName
isn't a String
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking better; just a few tweaks to make and it should be good to merge. Thanks! 🙂
package.json
Outdated
@@ -69,6 +69,7 @@ | |||
"url": "http://github.com/localForage/localForage/issues" | |||
}, | |||
"dependencies": { | |||
"jslint": "^0.12.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in devDependencies
, not dependencies
, as you don't need jslint
to run the repo.
That said: this shouldn't need to be here at all. If we're using a linter it should be eslint
, but we aren't using a linter (or, specifically, jslint
) anywhere in the codebase or this PR. Could you remove this line please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forget that this type of dependency came under dev dependencies.
test/test.config.js
Outdated
@@ -269,4 +269,16 @@ describe('Config API', function() { | |||
expect(configResult.toString()).to.be(error); | |||
done(); | |||
}); | |||
|
|||
it('should throw error on config.storeName not string', function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a wording suggestion here to clean up the test:
it('should throw error on config.storeName not string', function(done) { | |
it('should throw error if config.storeName is not a string', function() { |
(You also don't need to use done()
here since this isn't an asynchronous test.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Got it
@@ -269,4 +269,16 @@ describe('Config API', function() { | |||
expect(configResult.toString()).to.be(error); | |||
done(); | |||
}); | |||
|
|||
it('should throw error on config.storeName not string', function(done) { | |||
let configResult = localforage.config({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let configResult = localforage.config({ | |
const configResult = localforage.config({ |
package.json
Outdated
@@ -23,6 +23,7 @@ | |||
"test": "node -e \"require('grunt').cli()\" null test" | |||
}, | |||
"devDependencies": { | |||
"jslint": "^0.12.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the code using jslint
? I mentioned tooling like this belongs in devDependencies
if used, but I don't see it being used anywhere in this PR so I think it's best to remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I think now it good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it now ready to merge??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be patient; this is an open-source project and we don't always have the time to respond to PRs right away. I do check my issue queues/notifications but I can't get to things right away 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. No worry.
Fixes #947