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

Implement a dead code elimination friendly strategy #4

Merged
merged 2 commits into from
Jun 9, 2016
Merged

Conversation

mmun
Copy link
Member

@mmun mmun commented May 30, 2016

Instead of special casing if statements, feature flag call expressions
are now simply replaced by true or false literals when possible. This
lets you use feature flagging in more expressive ways by leaning on dead
code eliminiation in tools like Uglify. For example, now you can do

if (isEnabled('foo') || isEnabled('bar')) {
  // Uglify will be able to remove this code if both features are disabled
}

cc @mixonic @rwjblue @krisselden

mmun added 2 commits May 30, 2016 14:48
Instead of special casing `if` statements, feature flag call expressions
are now simply replaced by true or false literals when possible. This
lets you use feature flagging in more expressive ways by leaning on dead
code eliminiation in tools like Uglify. For example, now you can do

```js
if (isEnabled('foo') || isEnabled('bar')) {
  // Uglify will be able to remove this code if both features are disabled
}
```
@mmun
Copy link
Member Author

mmun commented May 30, 2016

http://marijnhaverbeke.nl/uglifyjs is useful if you want to experiment with the DCE

@rwjblue
Copy link
Member

rwjblue commented Jun 2, 2016

I'm concerned (slightly) that this is actually slightly different in the output for non-minified production builds of Ember. Specifically, today all disabled features are completely stripped but with this change they would not be stripped (both sides of the if would be included in the output).

I'm certain that @mmun thought of this, but I wanted to make sure that we are all on the same page and that this is not something we are concerned with....

@mmun
Copy link
Member Author

mmun commented Jun 2, 2016

@rwjblue I totally agree! We should tread carefully. Would be good to get Stef and Kris' thoughts as well.

@mixonic
Copy link
Member

mixonic commented Jun 5, 2016

💅 fancy

@Turbo87 Turbo87 mentioned this pull request Jun 9, 2016
@mmun mmun merged commit 3833c85 into master Jun 9, 2016
@mmun mmun deleted the dce-friendly branch June 9, 2016 16:47
@mmun
Copy link
Member Author

mmun commented Jun 9, 2016

@rwjblue and I were pretty comfortable merging as is. If any complications occur we can always implement a dual strategy that handles ifs explicitly again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants