esbuild icon indicating copy to clipboard operation
esbuild copied to clipboard

json loader should compile to `JSON.parse("<json text>")`

Open jfirebaugh opened this issue 2 years ago • 1 comments

JSON.parse is significantly faster than parsing object literals. The JSON loader could take advantage of this by compiling the default export to a JSON.parse of a string literal.

A potential disadvantage is that it would prevent tree-shaking unused portions of the object. I don't know if the esbuild implementation permits conditionally using JSON.parse or object literal depending on what gets tree shaken or not. If not, maybe using JSON.parse could be a configuration option?

At Figma we use loader: { '.json': 'text' } and do the JSON.parse ourself (in fact I think you set it up this way originally, for performance reasons I'm assuming), but this is less than ideal because it may get parsed more than once and apparently doesn't play nice with Jest.

jfirebaugh avatar Jul 22 '22 21:07 jfirebaugh

Interesting. Some thoughts:

  • Why does using loader: { '.json': 'text' } with JSON.parse cause it to be parsed more than once? It seems like that's all you're asking esbuild to do here, so I wanted to double-check that I'm understanding you correctly.

  • What about this doesn't play nice with Jest? Does Jest somehow not like string literals containing JSON? Or does the Jest sandbox not like calls to JSON.parse? Something else?

  • I assume you wouldn't want to do this for small JSON literals, or also some big JSON literals (e.g. a single long top-level string). I wonder what a good heuristic is for when doing this makes sense.

  • I also wonder what the time savings is in your case for your code with and without this optimization. Are we talking 5ms? 50ms? 500ms? I wouldn't want to take on this extra complexity without it being a significant win.

A potential disadvantage is that it would prevent tree-shaking unused portions of the object. I don't know if the esbuild implementation permits conditionally using JSON.parse or object literal depending on what gets tree shaken or not.

Perhaps. For myself/for the record, this code currently lives here:

https://github.com/evanw/esbuild/blob/fd13718c6195afb9e63682476a774fa6d4483be0/internal/bundler/linker.go#L1886-L1973

This is run during the linking phase for all data-style loaders (text, json, base64, etc.). What happens is this:

  1. If the module was imported as CommonJS, we generate the AST for module.exports = value and stop. No tree shaking occurs. Any named ESM import other than default will be linked at run-time by the ESM/CommonJS compatibility shim.

  2. Otherwise, we generate the AST for export default value. But in addition, we also essentially generate the AST for export let X = value.X for each top-level property X if value is an object literal.

  3. Then, after tree shaking occurs, any variables corresponding to top-level properties (e.g. export let X) that end up not being removed by tree shaking are considered live. The values for those live top-level properties in the default exported object literal are then substituted for a reference to the corresponding variable. The complexity of splitting this into two phases is necessary because tree shaking needs to happen to identify which variables are unused. This is handled by the code here (and is described more in the comments):

https://github.com/evanw/esbuild/blob/fd13718c6195afb9e63682476a774fa6d4483be0/internal/bundler/linker.go#L3953-L4013

So a simple thing to do could be to a) always convert the JSON AST for CommonJS modules and a) only convert the default export for ES modules after tree shaking if none of the top-level properties end up being used. But then you would presumably only want to do this if some heuristic check passes (e.g. the JSON is of sufficient length/complexity, such as not just a single string literal).

evanw avatar Jul 23 '22 02:07 evanw

Closing this issue due to lack of a follow-up reply.

evanw avatar Nov 27 '22 16:11 evanw