dataToNamedExports helper #29
Conversation
guybedford
force-pushed
the
dataToNamedExports
branch
from
3ed8336 to
80d57ea
Compare
|
Thanks for review on this - I've renamed to |
|
Looks good. If you want this to be released, the "compact" option should probably added to the readme. If this is still work in progress, just let me know. If we expect more than one option to be added, the options should probably become an options object. |
|
I've gone ahead and updated the implementation here to match rollup/rollup-plugin-json#41, so if you want you could update that PR to reference this utility now. I also caught a minor bug in the process that non legal keys could contain quote characters, so they now run through JSON.stringify themselves. I think it's nice to maintain the implementation in a single place here for all the data format plugins, so would like to get this merged, even if the JSON plugin itself doesn't use this. |
| compact: false, | ||
| indent: '\t', | ||
| preferConst: false, | ||
| objectShorthand: false |
src/dataToEsm.js
Outdated
|
|
||
| let namedExportCode = ''; | ||
| const defaultExportRows = []; | ||
| Object.keys( data ).forEach(key => { |
There was a problem hiding this comment.
Just a minor remark: Even though we do not need the keys variable any more, it might still make sense to use the more performant previous version of the loop instead.
There was a problem hiding this comment.
There's a bit of an issue with that as Buble doesn't support transforming for-of. I think we're ok when not in tight loops for this sort of optimization though.
There was a problem hiding this comment.
I was actually suggesting to use the exact code as used in the initial version—a classic for loop with an iterator variable. It's still the fastest way to iterate. for...of loops in TypeScript are only fast because due to our compilation target, it is transpiled to a classic for loop. Real for...of loops are actually quite a bit slower due to the hidden iterator evaluations anyway.
There was a problem hiding this comment.
Ah I completely forgot I did that. Sure.
test/test.js
Outdated
| }); | ||
|
|
||
| it( 'supports a compact argument', function () { | ||
| assert.equal( dataToEsm( { some: 'data', another: 'data' }, true ), 'export const some="data";export const another="data";export default{some,another};' ); | ||
| assert.equal( dataToEsm( { some: { deep: { object: 'definition', here: 'here' } }, another: 'data' }, true ), 'export const some={deep:{object:"definition",here:"here"}};export const another="data";export default{some,another};' ); | ||
| assert.equal( dataToEsm( { some: 'data', another: 'data' }, { compact: true, objectShorthand: true } ), 'export var some="data";export var another="data";export default{some,another};' ); |
There was a problem hiding this comment.
I would love to see the output of { compact: true, objectShorthand: false } tested as well as I think this is an important case considering backward compatibility.
There was a problem hiding this comment.
Sure I've included this test case.
lukastaegert
force-pushed
the
dataToNamedExports
branch
from
c9d30c7 to
5f40964
Compare
|
Apparently, |
|
@lukastaegert that looks more like a registry bug than an npm one, it may be worth copying it over to the registry repo rather - https://github.com/npm/registry and closing the npm issue. May well get a better response. |
|
Or maybe not - https://github.com/npm/registry/issues/312 :) |
|
Otherwise it may just be worth asking @Rich-Harris to do a publish here if the permissions aren't working for you. |
|
I've published a new version, added @guybedford as an owner, and removed-then-added @lukastaegert, so hopefully whatever gremlins were affecting npm are no longer an issue |
This is a helper I've found is needed by all the data plugins (eg yaml, json, toml) in order to output treeshakable data.
As far as I can tell the JSON plugin does currently support treeshaking in this way, but for example the YAML plugin doesn't seem to. Getting the logic on a shared helper like this seems like it would help ensure consistency here.