Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

BOM as a whitespace #722

Closed
wants to merge 1 commit into from

6 participants

@subzey

Due to changes in EcmaScript 5 (http://es5.github.com/#E , 15.10.2.12) Byte Order Mark character may be or may be not recognized as a whitespace depending on ES version implemented in browser.

For example, this code:

$.parseJSON("\uFEFF[]");

throws an error in older IEs while modern browsers returns array.

@sindresorhus sindresorhus commented on the diff
src/core.js
@@ -913,9 +913,21 @@ if ( jQuery.browser.webkit ) {
}
// IE doesn't match non-breaking spaces with \s
-if ( rnotwhite.test( "\xA0" ) ) {
- trimLeft = /^[\s\xA0]+/;
- trimRight = /[\s\xA0]+$/;
+if ( rnotwhite.test ( "\xA0" ) ) {

Wrong style. Please read the jQuery Core Style Guidelines.

Correct:
if ( rnotwhite.test("\xA0") ) {

Also applies to the other if statements.

The original is wrong too then ;)

@dmethvin Owner
dmethvin added a note

@rwldrn did we decide to remove the style exception for spaces around quotes within parens? TBH they have always confused me too; I know there was some recent discussion.

@rwaldron Collaborator
rwaldron added a note

@dmethvin Correct, we switched to no spaces around single string args.

@Krinkle
Krinkle added a note

There shouldn't be a space before the arguments list: ".test("

@rwaldron Collaborator
rwaldron added a note

Please update to match the latest style guide, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus

Can you file a ticket and add the ticket number here?

Please also provide a testcase.

Why are you adding this to trimRight? From what I understand the BOM is only at the beginning of a file.

You can make it a lot smaller by using the Regexp constructor instead.

Though this should probably be added to the AJAX method instead, since it only applies there.

@subzey

@sindresorhus,
thanks for your corrections, I'll correct the code style.
I'd file an issue to bugtracker, but dev.jquery.com is seems to be down.

BOM is historical name for ZWNBSP, this char actually may apper anywhere in string, not only in beginning.

I used inline regexps and if tree because of code execution speed: constructing string for RegExp would require array (btw, extra var in scope) and array operations.

@subzey

Thanks! Suppose, I wasn't attentive enough, sorry.

Would it better to delete this request and re-file this issue via bug tracker?

@dmethvin
Owner

Man that seems like a lot of code for this case, must be a smaller way to write it. What if we started with "\s" and then appended \xA0 and \xFEFF as needed, then used new RegExp on that?

@dmethvin
Owner

Oh, and we need a unit test for it.

If we know the problem browsers, we should add a comment for that too. Sounds like it's an oldIE issue only, so we may be able to remove it when we drop support for them.

@dmethvin
Owner

Since this pull request has been inactive for a while, I'm closing it.

@dmethvin dmethvin closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 1, 2012
  1. @subzey
This page is out of date. Refresh to see the latest.
Showing with 15 additions and 3 deletions.
  1. +15 −3 src/core.js
View
18 src/core.js
@@ -913,9 +913,21 @@ if ( jQuery.browser.webkit ) {
}
// IE doesn't match non-breaking spaces with \s
-if ( rnotwhite.test( "\xA0" ) ) {
- trimLeft = /^[\s\xA0]+/;
- trimRight = /[\s\xA0]+$/;
+if ( rnotwhite.test ( "\xA0" ) ) {

Wrong style. Please read the jQuery Core Style Guidelines.

Correct:
if ( rnotwhite.test("\xA0") ) {

Also applies to the other if statements.

The original is wrong too then ;)

@dmethvin Owner
dmethvin added a note

@rwldrn did we decide to remove the style exception for spaces around quotes within parens? TBH they have always confused me too; I know there was some recent discussion.

@rwaldron Collaborator
rwaldron added a note

@dmethvin Correct, we switched to no spaces around single string args.

@Krinkle
Krinkle added a note

There shouldn't be a space before the arguments list: ".test("

@rwaldron Collaborator
rwaldron added a note

Please update to match the latest style guide, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // BOM is not a whitespace in EcmaScript 3
+ if ( rnotwhite.test ("\uFEFF") ) {
+ trimLeft = /^[\s\xA0\uFEFF]+/;
+ trimRight = /[\s\xA0\uFEFF]+$/;
+ } else {
+ trimLeft = /^[\s\xA0]+/;
+ trimRight = /[\s\xA0]+$/;
+ }
+} else {
+ // BOM is not a whitespace in EcmaScript 3
+ if ( rnotwhite.test ( "\uFEFF" ) ) {
+ trimLeft = /^[\s\uFEFF]+/;
+ trimRight = /[\s\uFEFF]+$/;
+ }
}
// All jQuery objects should point back to these
Something went wrong with that request. Please try again.