Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle undefined expressions in treeshaking #2264

Merged
merged 2 commits into from
Jun 20, 2018

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Jun 13, 2018

Resolves #2223

This extends the UNDEFINED_EXPRESSION to return a undefined literal value which can be treeshaken.

@guybedford
Copy link
Contributor Author

This resolves #2223 as well.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Just some suggestions for the test. Looks good!

console.log('no');
if (z ? (console.log('a'), false) : (console.log('b'), false))
console.log('yes');
z = 1;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too happy the comma expression logs. They sure prevent the ternary operators from being tree-shaken but in the light of what you want to test, I would rather have simple direct tests such as

if (x) console.log('no');
if (!x) console.log('yes');

i.e. six simple test cases which are more readable and where it is easily clear what went wrong when the test turns red. It's also easy to overlook in your test that the second value in the comma expressions are not symmetric.

@lukastaegert lukastaegert added this to the 1.0.0 milestone Jun 16, 2018
@lukastaegert lukastaegert added this to In Review in Release 1.0.0 Jun 16, 2018
@lukastaegert lukastaegert moved this from In Review to In progress in Release 1.0.0 Jun 16, 2018
@guybedford guybedford force-pushed the undefined-expression-treeshake branch from 5e1dbca to 468b4d6 Compare June 17, 2018 12:02
@lukastaegert lukastaegert moved this from In progress to In Review in Release 1.0.0 Jun 18, 2018
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Thanks!

@lukastaegert lukastaegert modified the milestones: 1.0.0, 0.61.0 Jun 20, 2018
@lukastaegert lukastaegert merged commit 468b4d6 into master Jun 20, 2018
Release 1.0.0 automation moved this from In Review to Done Jun 20, 2018
@lukastaegert lukastaegert deleted the undefined-expression-treeshake branch June 20, 2018 07:22
calebeby pushed a commit to Pigmice2733/scouting-frontend that referenced this pull request Jun 21, 2018
This Pull Request updates dependency [rollup](https://github.com/rollup/rollup) from `v0.60.7` to `v0.61.1`



<details>
<summary>Release Notes</summary>

### [`v0.61.1`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#&#8203;0611)
[Compare Source](rollup/rollup@v0.61.0...697f36d)
*2018-06-21*
* Do not try to deconflict "undefined" ([#&#8203;2291](`rollup/rollup#2291))
* Properly track values for loop interator declarations and reassigned namespaces, add smoke test ([#&#8203;2292](`rollup/rollup#2292))

---

### [`v0.61.0`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#&#8203;0610)
[Compare Source](rollup/rollup@v0.60.7...v0.61.0)
*2018-06-20*
* Declare file dependencies via transform plugin hooks ([#&#8203;2259](`rollup/rollup#2259))
* Handle undefined values when evaluating conditionals ([#&#8203;2264](`rollup/rollup#2264))
* Handle known undefined properties when evaluating conditionals ([#&#8203;2265](`rollup/rollup#2265))
* Access watch events via the plugin context ([#&#8203;2261](`rollup/rollup#2261))
* Add option to suppress `__esModule` flag in output ([#&#8203;2287](`rollup/rollup#2287))
* Fix issue when re-declaring variables, track reassignments in more cases ([#&#8203;2279](`rollup/rollup#2279))
* Add VSCode debug settings ([#&#8203;2276](`rollup/rollup#2276))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 1.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants