Throw an error if `config.storeName` isn't a String #948

himanshu199728 · 2020-04-18T19:53:35Z

Fixes #947

tofumatt

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! 😄

tofumatt · 2020-04-19T02:33:13Z

src/localforage.js

-            return this._config[options];
-        } else {
-            return this._config;
+        } catch (err) {


We shouldn't catch any error here; instead this should throw as it did previously.

tofumatt · 2020-04-19T02:34:48Z

src/localforage.js

+                    if (i === 'storeName' && typeof options[i] !== 'string') {
+                        options[i] = options[i].replace(/\W/g, '_');
+                    } else {
+                        return new Error('storeName must be a string');


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).

tofumatt · 2020-04-19T02:36:17Z

src/localforage.js

-                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') {


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.

tofumatt · 2020-04-19T02:37:03Z

src/localforage.js

-            for (let i in options) {
-                if (i === 'storeName') {
-                    options[i] = options[i].replace(/\W/g, '_');
+        try {


I don't think this try/catch block here is helpful to add, as it will prevent unexpected errors from throwing.

tofumatt · 2020-04-19T02:37:31Z

src/localforage.js

@@ -314,7 +322,7 @@ class LocalForage {
        }

        function initDriver(supportedDrivers) {
-            return function() {
+            return function () {


Not sure why these code changes were made, but they're failing the linter; could you please revert these style/whitespace changes?

Got this due to correcting the indentation by some extension. I'll revert it back.

I think test cases in the test folder needed to be more organized in categories for manging.

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

I didn't get any update from you, sir.

tofumatt

This is looking better; just a few tweaks to make and it should be good to merge. Thanks! 🙂

tofumatt · 2020-04-27T07:54:36Z

package.json

@@ -69,6 +69,7 @@
    "url": "http://github.com/localForage/localForage/issues"
  },
  "dependencies": {
+    "jslint": "^0.12.1",


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?

Sorry, I forget that this type of dependency came under dev dependencies.

tofumatt · 2020-04-27T07:56:47Z

test/test.config.js

@@ -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) {


Just a wording suggestion here to clean up the test:

Suggested change

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.)

tofumatt · 2020-04-27T07:57:06Z

test/test.config.js

@@ -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({


Suggested change

let configResult = localforage.config({

const configResult = localforage.config({

test/test.config.js

tofumatt · 2020-04-27T10:03:06Z

package.json

@@ -23,6 +23,7 @@
    "test": "node -e \"require('grunt').cli()\" null test"
  },
  "devDependencies": {
+    "jslint": "^0.12.1",


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.

Done. I think now it good to go.

Is it now ready to merge??

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 🙂

Ok. No worry.

himanshu199728 added 2 commits April 19, 2020 01:08

storeName value check

2c8475b

try catch block at config value set function

a633011

tofumatt requested changes Apr 19, 2020

View reviewed changes

tofumatt changed the title ~~Check fix of storeName~~ Throw an error if config.storeName isn't a String #947 Apr 19, 2020

tofumatt changed the title ~~Throw an error if config.storeName isn't a String #947~~ Throw an error if config.storeName isn't a String Apr 19, 2020

config.storeName check fix, intendation fixed

1c85180

himanshu199728 requested a review from tofumatt April 19, 2020 11:23

tofumatt requested changes Apr 27, 2020

View reviewed changes

unwanted code clean, package.json updated

1798dc0

tofumatt reviewed Apr 27, 2020

View reviewed changes

package.json update

7f2a0f8

himanshu199728 requested a review from tofumatt April 27, 2020 10:36

himanshu199728 added 2 commits May 3, 2020 19:24

end .

6622e72

Nothing new

b5e411a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw an error if `config.storeName` isn't a String #948

Throw an error if `config.storeName` isn't a String #948

himanshu199728 commented Apr 18, 2020 •

edited by tofumatt

tofumatt left a comment

tofumatt Apr 19, 2020

tofumatt Apr 19, 2020

tofumatt Apr 19, 2020

tofumatt Apr 19, 2020

tofumatt Apr 19, 2020

himanshu199728 Apr 19, 2020 •

edited

himanshu199728 Apr 19, 2020 •

edited

himanshu199728 Apr 19, 2020 •

edited

himanshu199728 Apr 25, 2020

tofumatt left a comment

tofumatt Apr 27, 2020

himanshu199728 Apr 27, 2020

himanshu199728 Apr 27, 2020

tofumatt Apr 27, 2020

himanshu199728 Apr 27, 2020 •

edited

tofumatt Apr 27, 2020

tofumatt Apr 27, 2020

himanshu199728 Apr 27, 2020

himanshu199728 Apr 27, 2020 •

edited

himanshu199728 May 1, 2020

tofumatt May 1, 2020

himanshu199728 May 1, 2020

	it('should throw error on config.storeName not string', function(done) {
	it('should throw error if config.storeName is not a string', function() {

	let configResult = localforage.config({
	const configResult = localforage.config({

Throw an error if config.storeName isn't a String #948

Are you sure you want to change the base?

Throw an error if config.storeName isn't a String #948

Conversation

himanshu199728 commented Apr 18, 2020 • edited by tofumatt

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshu199728 Apr 19, 2020 • edited

Choose a reason for hiding this comment

himanshu199728 Apr 19, 2020 • edited

Choose a reason for hiding this comment

himanshu199728 Apr 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshu199728 Apr 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshu199728 Apr 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Throw an error if `config.storeName` isn't a String #948

Throw an error if `config.storeName` isn't a String #948

himanshu199728 commented Apr 18, 2020 •

edited by tofumatt

himanshu199728 Apr 19, 2020 •

edited

himanshu199728 Apr 19, 2020 •

edited

himanshu199728 Apr 19, 2020 •

edited

himanshu199728 Apr 27, 2020 •

edited

himanshu199728 Apr 27, 2020 •

edited