This repository has been archived by the owner. It is now read-only.

Use fromCodePoint to convert high value unicode entities #243

Merged
merged 4 commits into from Jan 2, 2017

Conversation

Projects
None yet
4 participants
@ryanjduffy
Copy link
Contributor

ryanjduffy commented Nov 29, 2016

Q A
Bug fix? yes
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? no
Tests added/pass? yes
Fixed tickets comma-separated list of tickets fixed by the PR, if any
License MIT

High code point Unicode entity references are being truncated because String.fromCharCode can only return a 2-byte character whereas String.fromCodePoint can return a 4-byte character. Interestingly, the existing test case includes an appropriately high code point reference but the expected result is the truncated value and not the truly expected value.

With this change, 💩 is correctly converted to 💩 instead of (which would be the value of ).

@danez

This comment has been minimized.

Copy link
Member

danez commented Dec 1, 2016

Thanks for spotting this. Awesome.

You added the polyfill as devDependency although it probably should be a dependency. But even then it won't work I think, because we use rollup to bundle babylon.

Other option would be to drop support for node < 4.
@hzoo @DrewML ideas?

@danez danez added the Tag: Bug Fix label Dec 1, 2016

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Dec 1, 2016

Wouldn't rollup just bundle the polyfill in?

Yeah we can drop it but not in this pr, would need to plan that out

@danez

This comment has been minimized.

Copy link
Member

danez commented Dec 1, 2016

not sure how the bundling works, would need to check. But then npm/yarn couldn't do deduplicating correctly as we hard inline it and it also seems to modify globals.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Dec 1, 2016

Oh - then we need to use a version that gives a reference rather than the actual polyfill - like this kind of thing

var fromCodePoint = require('fromCodePoint');
fromCodePoint();
@ryanjduffy

This comment has been minimized.

Copy link
Contributor Author

ryanjduffy commented Dec 5, 2016

If the plan is to drop support for node < 4 anyway, what do you think of including an inline copy of the polyfill (the fn without updating String, that is)? The polyfill code hasn't been updated in 2.5 years so there's not much risk of getting out of sync between now and when you'd be able to remove it. Seems like a safe and minimally intrusive alternative.

ryanjduffy added some commits Dec 8, 2016

copy and adapt fromCodePoint into String.fromcodepoint
In order to avoid modifying String as the polyfill does, I've copied
the source from the polyfill and adapted it return the polyfill
function if the native version does not exist. Once support for node
versions that lack fromCodePoint is dropped, this polyfill can be
removed.
@ryanjduffy

This comment has been minimized.

Copy link
Contributor Author

ryanjduffy commented Dec 8, 2016

Went ahead and made a copy of the polyfill and dropped the dependency. Made the necessary changes to adapt it to the babylon lint settings and avoid updating String. Let me know what you think.

cc: @mathiasbynens - fyi on copying String.fromcodepoint (and thx for the polyfill!).

@mathiasbynens

This comment has been minimized.

Copy link
Contributor

mathiasbynens commented Dec 8, 2016

LGTM! Thanks for the CC, @ryanjduffy — I appreciate the heads-up 👍

Ref. babel/babel#4315 for dropping Node.js < 4 (in which case the polyfill can be removed).

@mathiasbynens

This comment has been minimized.

Copy link
Contributor

mathiasbynens commented Dec 8, 2016

Btw, since the goal is to decode HTML entities, consider using a library like he.

@danez

danez approved these changes Dec 9, 2016

@danez danez merged commit 1c13800 into babel:master Jan 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.