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

IE 7 throws error if number, or string starting with number, is used as key #40

Closed
pauldwaite opened this issue Mar 16, 2012 · 13 comments
Closed

Comments

@pauldwaite
Copy link

If you attempt to use a number, or a string starting with a number, as a key, then IE 7 will throw an error, e.g.

This name may not begin with the '1' character

I’m not sure if using numbers or strings starting with numbers as keys is meant to be supported, but it seems to work in IE 8 and other browsers.

Test page:

<!doctype html>
<html lang="en">
<head>
    <meta charset="utf-8">

    <script type="text/javascript" src="store.js"></script>

    <title>Test store.js</title>
</head>
<body>

<button onclick="store.set('string', 'value');"><code>store.set('string', 'value')</code></button>

<button onclick="store.set('123456', 'value');"><code>store.set('123456', 'value')</code></button>

<button onclick="store.set(123456, 'value');"><code>store.set(123456, 'value')</code></button>

<br>
<br>

<button onclick="alert(store.get('string', 'value'));"><code>store.get('string', 'value')</code></button>

<button onclick="alert(store.get('123456', 'value'));"><code>store.get('123456', 'value')</code></button>

<button onclick="alert(store.get(123456, 'value'));"><code>store.get(123456, 'value')</code></button>

</body>

</html>

I’ve worked around the issue by patching store.js to append 'string_' to the key within the get(), set() and remove() functions passed to withIEStorage(), but I’m not sure if this is optimal.

@marcuswestin
Copy link
Owner

Fascinating! I think prefixing IE keys with "_" is a perfectly reasonable
thing to do. If you feel up to patching it I'd definitely merge in a pull
request.

Cheers!
Marcus

On Fri, Mar 16, 2012 at 5:04 AM, Paul D. Waite <
reply@reply.github.com

wrote:

If you attempt to use a number, or a string starting with a number, as a
key, then IE 7 will throw an error, e.g.

This name may not begin with the '1' character

Im not sure if using numbers or strings starting with numbers as keys is
meant to be supported, but it seems to work in IE 8 and other browsers.

Test page:

   <!doctype html>
   <html lang="en">
   <head>
           <meta charset="utf-8">

           <script type="text/javascript" src="store.js"></script>

           <title>Test store.js</title>
   </head>
   <body>

   <button onclick="store.set('string',

'value');">store.set('string', 'value')

   <button onclick="store.set('123456',

'value');">store.set('123456', 'value')

   <button onclick="store.set(123456,

'value');">store.set(123456, 'value')

   <br>
   <br>

   <button onclick="alert(store.get('string',

'value'));">store.get('string', 'value')

   <button onclick="alert(store.get('123456',

'value'));">store.get('123456', 'value')

   <button onclick="alert(store.get(123456,

'value'));">store.get(123456, 'value')

   </body>

   </html>

Ive worked around the issue by patching store.js to append 'string_' to
the key within the get(), set() and remove() functions passed to
withIEStorage(), but Im not sure if this is optimal.


Reply to this email directly or view it on GitHub:
#40

My code http://github.com/marcuswestin
My latest http://twitter.com/marcuswestin

@pauldwaite
Copy link
Author

Cheers Martin, I’ll get something together for your consideration next week.

Yours sincerely,
Paul D. Waite

On 17 Mar 2012, at 1:15 a.m., Marcus Westin wrote:

Fascinating! I think prefixing IE keys with "_" is a perfectly reasonable
thing to do. If you feel up to patching it I'd definitely merge in a pull
request.

Cheers!
Marcus

On Fri, Mar 16, 2012 at 5:04 AM, Paul D. Waite <
reply@reply.github.com

wrote:

If you attempt to use a number, or a string starting with a number, as a
key, then IE 7 will throw an error, e.g.

This name may not begin with the '1' character

Im not sure if using numbers or strings starting with numbers as keys is
meant to be supported, but it seems to work in IE 8 and other browsers.

Test page:

  <!doctype html>
  <html lang="en">
  <head>
          <meta charset="utf-8">

          <script type="text/javascript" src="store.js"></script>

          <title>Test store.js</title>
  </head>
  <body>

  <button onclick="store.set('string',

'value');">store.set('string', 'value')

  <button onclick="store.set('123456',

'value');">store.set('123456', 'value')

  <button onclick="store.set(123456,

'value');">store.set(123456, 'value')

  <br>
  <br>

  <button onclick="alert(store.get('string',

'value'));">store.get('string', 'value')

  <button onclick="alert(store.get('123456',

'value'));">store.get('123456', 'value')

  <button onclick="alert(store.get(123456,

'value'));">store.get(123456, 'value')

  </body>

  </html>

Ive worked around the issue by patching store.js to append 'string_' to
the key within the get(), set() and remove() functions passed to
withIEStorage(), but Im not sure if this is optimal.


Reply to this email directly or view it on GitHub:
#40

My code http://github.com/marcuswestin
My latest http://twitter.com/marcuswestin


Reply to this email directly or view it on GitHub:
#40 (comment)

@marcuswestin
Copy link
Owner

Fixed in d0a5e64

@pauldwaite
Copy link
Author

Now that’s service, cheers Marcus.

Yours sincerely,
Paul Waite

On 22 Mar 2012, at 12:34 AM, Marcus Westin reply@reply.github.com wrote:

Fixed in d0a5e64


Reply to this email directly or view it on GitHub:
#40 (comment)

@marcuswestin
Copy link
Owner

Happy to help :)

-- while mobile

On Mar 26, 2012, at 11:29 PM, "Paul D. Waite"reply@reply.github.com wrote:

Now that’s service, cheers Marcus.

Yours sincerely,
Paul Waite

On 22 Mar 2012, at 12:34 AM, Marcus Westin reply@reply.github.com wrote:

Fixed in d0a5e64


Reply to this email directly or view it on GitHub:
#40 (comment)


Reply to this email directly or view it on GitHub:
#40 (comment)

@mferretti
Copy link

Hi ,

I just tested the library ( GR8 job ! ) and have something on this : my IE7 complains about anything that's not alphanumeric ( eg : /,(,% etc ) . I came across this issue trying to port saiku-ui to IE7 ( localStorage problem ) . I am currently working around the issue by adding a .replace(/\W/g, "") to the key when returning from ieKeyFix(key). Still need to extensively test it but am 85% sure that fixes all of the other cases

@marcuswestin
Copy link
Owner

Aha! Nice find @mferretti!

I'm hesitant to simply remove them all since it could result in key collisions that would be very subtle and hard to debug (only keys with obscure combinations of characters, and in IE7 only).

I wonder if there's a straight forward encoding that we could apply to all keys. Have you tried using encodeURIComponent? E.g.

return encodeURIComponent('_'+key)

I would love the help with adding test cases to test.html that tests these edge-case key :)

@mferretti
Copy link

well the

return encodeURIComponent('_'+key)

would translate the string adding chars like '%' which, again, would return an error.
Anyways, I will give it a shot tomorrow morning when back @ my dev env :)

@marcuswestin
Copy link
Owner

Ah, of course.

I wonder if an encodeURIComponent that then special cases + and % would
suffice.

Anyways - looking forward to see what you come up with!

On Tue, Jul 10, 2012 at 11:37 AM, Marco Ferretti <
reply@reply.github.com

wrote:

well the

return encodeURIComponent('_'+key)

would translate the string adding chars like '%' which, again, would
return an error.
Anyways, I will give it a shot tomorrow morning when back @ my dev env :)


Reply to this email directly or view it on GitHub:
#40 (comment)

@mferretti
Copy link

ok,
so far I was able to spot the following list of chars that are not allowed
' ' ( space ) ,/,%,(,)

this means that encodeURIComponent is not sufficient.
the function ieKeyFix(key) now looks like ( in my env )

        function ieKeyFix(key) {
            // In IE7, keys may not begin with numbers.
            // See https://github.com/marcuswestin/store.js/issues/40#issuecomment-4617842
            //remove spaces,remove %
            key = key.replace(/\s/g,'').replace(/%/g,'');
            //remove /
            key = key.replace(/\//g,'');
            //remove (
            key = key.replace(/\(/g,'');
            //remove )
            key = key.replace(/\)/g,'');


            return '_'+key; 
        }

which is not THAT far from

    return '_'+key.replace(/\W/g, "") ;

@marcuswestin
Copy link
Owner

Nice work! I would gladly accept a patch for this - think I would prefer to
replace with something like '$' at least to give a clue that they are being
replaced, and perhaps add a note to README about this.

Also, we should add a Contributors section to README - feel free to add
yourself there :)

On Wed, Jul 11, 2012 at 1:30 AM, Marco Ferretti <
reply@reply.github.com

wrote:

ok,
so far I was able to spot the following list of chars that are not allowed
' ' ( space ) ,/,%,(,)

this means that encodeURIComponent is not sufficient.
the function ieKeyFix(key) now looks like ( in my env )

                function ieKeyFix(key) {
                        // In IE7, keys may not begin with numbers.
                        // See
https://github.com/marcuswestin/store.js/issues/40#issuecomment-4617842
            //remove spaces,remove %
            key = key.replace(/\s/g,'').replace(/%/g,'');
            //remove /
            key = key.replace(/\//g,'');
            //remove (
            key = key.replace(/\(/g,'');
            //remove )
            key = key.replace(/\)/g,'');


                        return '_'+key;
                }

which is not THAT far from

        return '_'+key.replace(/\W/g, "") ;

Reply to this email directly or view it on GitHub:
#40 (comment)

@mferretti
Copy link

mmm ... looks like you cannot use any "special" char here

I came up with this test git://gist.github.com/3097343.git : basically testing all the ASCII table.

The result is that in IE7 the following chars cannot be used within a key ( skipping non printable chars ) :

code 9 throwed an This name may not contain the ' ' character: _key--> <--

code 10 throwed an This name may not contain the ' ' character: _key--> <--

code 11 throwed an This name may not contain the ' ' character: _key--> <--

code 12 throwed an This name may not contain the ' ' character: _key--> <--

code 13 throwed an This name may not contain the ' ' character: _key--> <--

code 32 throwed an This name may not contain the ' ' character: _key--> <--

code 33 throwed an This name may not contain the '!' character: _key-->!<--

code 34 throwed an This name may not contain the '"' character: _key-->"<--

code 35 throwed an This name may not contain the '#' character: _key-->#<--

code 36 throwed an This name may not contain the '$' character: _key-->$<--

code 37 throwed an This name may not contain the '%' character: _key-->%<--

code 38 throwed an This name may not contain the '&' character: _key-->&<--

code 39 throwed an This name may not contain the ''' character: _key-->'<--

code 40 throwed an This name may not contain the '(' character: _key-->(<--

code 41 throwed an This name may not contain the ')' character: _key-->)<--

code 42 throwed an This name may not contain the '_' character: key--><--

code 43 throwed an This name may not contain the '+' character: _key-->+<--

code 44 throwed an This name may not contain the ',' character: _key-->,<--

code 47 throwed an This name may not contain the '/' character: _key-->/<--

code 58 throwed an This name may not contain the ':' character: _key-->:<--

code 59 throwed an This name may not contain the ';' character: _key-->;<--

code 60 throwed an This name may not contain the '<' character: _key--><<--

code 61 throwed an This name may not contain the '=' character: _key-->=<--

code 62 throwed an This name may not contain the '>' character: _key-->><--

code 63 throwed an This name may not contain the '?' character: _key-->?<--

code 64 throwed an This name may not contain the '@' character: _key-->@<--

code 91 throwed an This name may not contain the '[' character: _key-->[<--

code 92 throwed an This name may not contain the '' character: _key--><--

code 93 throwed an This name may not contain the ']' character: _key-->]<--

code 94 throwed an This name may not contain the '^' character: _key-->^<--

code 96 throwed an This name may not contain the '' character: _key--><--

code 123 throwed an This name may not contain the '{' character: _key-->{<--

code 124 throwed an This name may not contain the '|' character: _key-->|<--

code 125 throwed an This name may not contain the '}' character: _key-->}<--

code 126 throwed an This name may not contain the '' character: _key--><--

The bottom line is that all the "special chars" cannot be used thus, if we fear the collision, I suggest to go for a substitution table that does something like :

col for ":"
til for "~"

and so on ....

What do you think ?

@mferretti
Copy link

I have come up with a ( hopefully ) more elegant solution.
the key is to define an array of forbidden char codes and then check the chars of the keys against it. If the element is present then we substitute the forbidden char with "forbiddenCharCode" : this ensures the key is usable and should prevent key collisions due to the removed chars.

What happens is ( eg ) :

'something{note_the_graph' --> 'something_123_note_the_graph'

if we're not hitting some max length here I think we can safely assume this is going to work :)

Under the hood :

I am testing if key != key.replace(/\W/g, "") , if that happens then we need to check the key and change the offending chars. Also, I added ( copied from mozilla ) an indexOf to the array ( IE doesn't support it I think until version 9 or something ) so that we can make things work a little faster.

now .... how do you want me to submit the patch ? shall i fork and push or just send it over to you via email/gist ?

marcuswestin added a commit that referenced this issue Jul 13, 2012
…which are not allowed. This addresses issue #40 and merges the relevant changes by @mferretti in PR #49
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

3 participants