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

Now longer create a fake AST but let rollup handle it - #41

Merged
lukastaegert merged 7 commits into
masterfrom
real-ast
May 11, 2018
Merged

Now longer create a fake AST but let rollup handle it#41
lukastaegert merged 7 commits into
masterfrom
real-ast

Conversation

@lukastaegert

Copy link
Copy Markdown
Member

This will resolved some issues encountered when implementing rollup/rollup#2167

Previously, rollup-plugin-json went to great lengths to create a "fake AST", presumably to avoid an unnecessary parse. This fake AST, however, would pretend all values (even objects!) were literals with value null while only the actual rendering would "fix" the generated code.

Now that tree-shaking for conditionals is about to be extended, this obviously causes a lot of issues. The approach taken here is to not generate an AST at all but just do the conversion to JavaScript and let Rollup generate a valid AST itself. In combination with rollup/rollup#2167, this will enable things like

// config.json
{
  "development": false,
  "nested": {
    "value": 3
  }
}

// main.js
import {development, nested} from './config.json';

if (development) {
  console.log('development');
}
if (nested.value > 2) {
  console.log('value is big!');
}

// output
{
  console.log('value is big!');
}

Comment thread src/index.js Outdated
defaultExportRows.push(`${key}: ${key}`);
namedExportCode += `export ${declarationType} ${key} = ${JSON.stringify(data[key])};\n`;
} else {
defaultExportRows.push(`"${key}": ${JSON.stringify(data[key])}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a lot nicer than what I did in https://github.com/rollup/rollup-pluginutils/pull/29/files actually.

@guybedford guybedford left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks great and nice to have JSON treeshaking!

I may well update https://github.com/rollup/rollup-pluginutils/pull/29/files to merge in this logic and reference the utility function from here when I get back to the plugin stuff.

Unrelated - I'd be interested to know why it's necessary to return an empty map with a mappings key? Was that related to the AST side of things perhaps and we can remove it now? It would be good to understand the use case from a plugin authoring perspective. Completely understand if that is lost to history too... although then perhaps best to just remove it entirely?

@lukastaegert

Copy link
Copy Markdown
Member Author

@guybedford I have finally updated the plugin to use dataToEsm. Note that the function produces some weird (or at least maybe not 100% consistent) formatting that we may want to improve in rollup-pluginutils, cf. the test samples in the samples/form folder:

  • There is a linebreak between array elements but not in front of the first or after the last element; maybe we want to change this one way or another
  • Unless an indent is specified, there is no space after an opening { or [ but a space before closing } or ]. This looks a little unintended.

I must admit I did not investigate further in rollup-pluginutils as to the cause. Thanks a lot for taking a look at the issues there BTW! I wonder which of the two updates you reverted was the original culprit. Does not feel good to have to be careful about updates but then again I probably should not have done any updates on a minor version, at least no major ones.

I also updated the dependencies of rollup-plugin-json but this time it is going to be a major version anyway.

I'd be interested to know why it's necessary to return an empty map with a mappings key

This is following the convention described here: https://github.com/rollup/rollup/wiki/Plugins#conventions

Basically we signal that a source map does not make sense (because we are too lazy to calculate one...). I guess this is acceptable for pure data exports.

@guybedford

Copy link
Copy Markdown

@lukastaegert thanks for the update and on the source maps stuff.

So it turns out this is the way that the third-party package we're using handles whitespace (from the Yaml / Toml plugin I believe). I couldn't actually find any alternatives at all in the ecosystem so ended up rewriting the logic in https://github.com/rollup/rollup-pluginutils/pull/31/files.

@lukastaegert

Copy link
Copy Markdown
Member Author

I guess a rewrite is ok here. Of course for the JSON use case, a good old JSON.stringify(data, null, indent) would have sufficed :). But I guess there are some edge cases that are probably handled better by this logic.

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