Skip to content
This repository was archived by the owner on Aug 4, 2021. It is now read-only.

add support for edge cases - #64

Merged
lukastaegert merged 5 commits into
rollup:masterfrom
LongTengDao-fork-A:master
Jun 4, 2019
Merged

add support for edge cases#64
lukastaegert merged 5 commits into
rollup:masterfrom
LongTengDao-fork-A:master

Conversation

@LongTengDao

@LongTengDao LongTengDao commented May 28, 2019

Copy link
Copy Markdown
Contributor
  • U+2028 U+2029
  • -Infinity -0
  • export default Date/RegExp
  • export default <space> (even when compact) value (startsWith letter/number)
  • export default { key: (when compact) value }
  • fix empty key/identifier error (which already has test and should already be fixed)

* U+2028 U+2029
* -Infinity -0
* export default Date/RegExp
* export default + space (even when compact) + value (startsWith letter/number)

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really nice collection of edge cases! Just some minor comments.

Comment thread src/dataToEsm.ts Outdated
export type Indent = string | null | undefined;

function stringify (obj :any) :string {
return (JSON.stringify(obj) || 'undefined').replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the fallback undefined good for? If I remove it, no test turns red and I could not come up with a situation where JSON.stringify would return a falsy value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe the regular expressions could be combined? Then it would be easy to add more characters to be escaped:

JSON.stringify(obj).replace(/[\u2028\u2029]/g, char => `\\u${char.charCodeAt(0).toString(16))}`;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see JSON.stringify(undefined) returns undefined. So maybe just add a test.

@LongTengDao LongTengDao Jun 1, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see JSON.stringify(undefined) returns undefined. So maybe just add a test.

@lukastaegert Also JSON.stringify(function(){}). I added tests as your notice~

BTW: I can't figure out why the bug of empty key/identifier error happen, which should already be fixed even with test before this PR.

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, thanks a lot!

@lukastaegert
lukastaegert merged commit c41c17b into rollup:master Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants