feat(pluginutils): support bigint and symbol in dataToEsm #1047
Conversation
LongTengDao
changed the title
| @@ -2,6 +2,13 @@ import test from 'ava'; | |||
|
|
|||
| import { dataToEsm } from '../'; | |||
|
|
|||
| test('support bigint and symbol', (t) => { | |||
There was a problem hiding this comment.
please split these into separate tests. (that may seem pedantic, but this PR essentially adds two features in support for two different entities)
| @@ -2,6 +2,13 @@ import test from 'ava'; | |||
|
|
|||
| import { dataToEsm } from '../'; | |||
|
|
|||
| test('support bigint and symbol', (t) => { | |||
| t.is( | |||
| dataToEsm({ bigint: 0, symbol: Symbol.for('') }), | |||
There was a problem hiding this comment.
I'd like to see a few different values (within the same t test func) tried here for sanity check
| @@ -4,8 +4,8 @@ import makeLegalIdentifier from './makeLegalIdentifier'; | |||
|
|
|||
| export type Indent = string | null | undefined; | |||
|
|
|||
| function stringify(obj: unknown): string { | |||
| return (JSON.stringify(obj) || 'undefined').replace( | |||
| function stringify(val: boolean | number | string): string { | |||
There was a problem hiding this comment.
why is this change necessary?
There was a problem hiding this comment.
because rest types (undefined object symbol bigint) already and should be handled in serialize, I think this will make the code more strong.
if it's not necessary, this change could be reverted.
There was a problem hiding this comment.
I reverted this change for CI undue error.
shellscape
changed the title
|
Looks like there are some failures caused by these changes in CI. Please have a look |
shellscape
changed the title
shellscape
changed the title
|
Closing as abandoned. The error is visible here: https://github.com/rollup/plugins/runs/5281573593?check_suite_focus=true#step:7:241. If you get around to cleaning that up, please update your fork and give us a ping. |
|
@shellscape Oh, sorry, I missed. The typo error has been corrected. |
|
Thanks for updating it! |
|
Looks like there is a failing test to take a look at. |
|
@shellscape Thanks for your careful guidance |
Rollup Plugin Name:
pluginutilsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
bigintundefinedbigintSymbol.for()undefinedSymbol.for()