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

BOM as a whitespace #722

Closed
wants to merge 1 commit into from
Closed

BOM as a whitespace #722

wants to merge 1 commit into from

Conversation

subzey
Copy link

@subzey subzey commented Apr 1, 2012

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.

if ( rnotwhite.test( "\xA0" ) ) {
trimLeft = /^[\s\xA0]+/;
trimRight = /[\s\xA0]+$/;
if ( rnotwhite.test ( "\xA0" ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong style. Please read the jQuery Core Style Guidelines.

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

Also applies to the other if statements.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original is wrong too then ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sindresorhus
Copy link
Contributor

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
Copy link
Author

subzey commented Apr 1, 2012

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

@sindresorhus
Copy link
Contributor

Use http://bugs.jquery.com

@subzey
Copy link
Author

subzey commented Apr 1, 2012

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

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

@dmethvin
Copy link
Member

dmethvin commented Apr 2, 2012

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
Copy link
Member

dmethvin commented Apr 2, 2012

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
Copy link
Member

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

@dmethvin dmethvin closed this May 27, 2012
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants