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

Reimagine how the babel transform works #53

Merged
merged 6 commits into from
Aug 13, 2019
Merged

Reimagine how the babel transform works #53

merged 6 commits into from
Aug 13, 2019

Conversation

achambers
Copy link
Collaborator

@achambers achambers commented Aug 9, 2019

This PR is part of #36. It is a reimagining of the babel transform that is used to convert the variation JS helper in to code that references the launch darkly service. This new approach somewhat simplifies the transform and also the reasoning on how it works.

Historical context

The original babel transform largely worked fine, however, it always had one lingering flaw, and that was it was tricky to detect when a variation helper was used inside a computed property. The reason we want to do this is so that we can auto insert the feature flags as dependent keys on the computed so that it will recalculate if the feature flag values change. As an example, the babel transform should take this:

foo: computed(function() {
  return variation('myflag');
})

and convert it to this:

foo: computed('launchDarkly.myflag', function() {
  return this.get('launchDarkly.myflag');
})

The reason this was tricky was that, because of how babel transforms are run, in a non deterministic order, it's very hard to be able to know what state the code will be in when this transform is running. For instance, firstly, depending on what version of Ember is being used, has the user imported computed from the old module import shims (ember-computed) or from the new ones (@ember/object)? Secondly, is the code still using the import or has the transform to transpile it in to Ember.computed run already?

Basically, depending on which version of Ember was used, and depending on how lucky you were when the transform ran, sometimes it would add the keys, sometimes it wouldn't.

So, we needed to find a more easy to reason about approach to signalling that a variation helper was used inside a computed property.

This is the main goal of this PR. The secondary goal is to simplify the transform to make it easier to understand what is going on.

New approach

The new approach as somewhat sacrificed the developer aesthetic of magically inserting the CP dependent keys but has drastically increased the reliability of the transform, increased the confidence we have of shipping it and removed ambiguity around which version of Ember is being used.

The new approach has introduced a second export, computedWithVariation. Having this explicit export allows us to signal to the transform that we need to add the dependent keys and we can be pretty sure that no other transform has modified this part of the code before us.

The intention is that users will use this instead of the regular computed provided by Ember, when they want to use a variation inside a computed property and have the dependent keys automatically added. This sounds like a lot of mental overhead but in reality it's very little. Here's why...

If the user doesn't care about recomputing the CP when a flag changes then there is no need to think about computedWithVariation. From a production perspective, users would only care about this if they have marked a flag as a "streaming" flag. However, from a development perspective, it is useful for flag changes to take effect when enabling/disabling them from the JS console. Still, it's not needed if the user doesn't care about this.

If the user does want their CP to be recomputed then they can simply alias computedWithVariation as computed and use it anywhere in the file that they'd use the Ember provided computed. This is because computedWithVariation is simply a re-export of computed from @ember/object. We just need it to signal to the transpiler that we should be looking for variation calls inside it and adding the flags as dependent keys.

So, users can simply do this:

import { computedWithVariation as computed } from 'ember-launch-darkly';

and they can use computed as they normally would. If they remove feature flags from that code then it's simply a matter of replacing that import with the standard @ember/object import. Even if they forget it won't matter.

Another bonus that this PR adds is messaging to the user if they don't include the babel plugin but do reference the variation helper. Before this PR, variation was a "fake" import that was replaced by the launch darkly service code. This meant that if the user did not include the babel transform then the code would not compile because it couldn't find a variation import from ember-launch-darkly. However, this PR now adds a variation export that simply throws an error with some useful messaging for the user pointing them to the docs. This will only fire if the user has referenced the variation helper and not included the babel transform.

Caveats with babel transform

Even with the new transform, there are a couple of caveats whereby the variation helper doesn't work. These are now covered in the README so users can be aware.

In short, the variation helper won't work when used in objects that don't have an owner (as we need to get access to the injected launchDarkly service), it won't work in native classes and we don't, yet, supply an @computedWithVariation decorator.

@achambers achambers self-assigned this Aug 9, 2019
README.md Outdated Show resolved Hide resolved
testem.js Outdated Show resolved Hide resolved
@patcoll
Copy link
Collaborator

patcoll commented Aug 12, 2019

@achambers Looks good, I see that the test-node tests are failing for me locally. We should probably include those as a part of the Travis test script 😄

@achambers
Copy link
Collaborator Author

@achambers Looks good, I see that the test-node tests are failing for me locally. We should probably include those as a part of the Travis test script 😄

On it. Thanks.

This helper eas broken when the version was updated. This just updates
it to work again. These routes will be removed as they add very little
value. Might tackle this separetly though, as a part of moving to
ember-addon-docs or something
@achambers achambers merged commit 666c5de into adopted-ember-addons:master Aug 13, 2019
@achambers achambers deleted the new-babel-transform-approach branch August 13, 2019 11:49
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.

2 participants