-
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?
Changes from 3 commits
2c8475b
a633011
1c85180
1798dc0
7f2a0f8
6622e72
b5e411a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Just a wording suggestion here to clean up the test:
Suggested change
(You also don't need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Got it |
||||||
let configResult = localforage.config({ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
description: 'The offline datastore for my cool app', | ||||||
driver: localforage.driver(), | ||||||
name: 'My Cool App', | ||||||
storeName: 123, | ||||||
version: 2.0 | ||||||
}); | ||||||
expect(configResult).to.be(new Error('storeName must be a string')); | ||||||
done(); | ||||||
tofumatt marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}); | ||||||
}); |
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
, notdependencies
, as you don't needjslint
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.