Skip to content

Commit

Permalink
fix(features): remove nested feature flags limitation (#1473)
Browse files Browse the repository at this point in the history 
* refactor: use `this` instead of `state`

* chore: init stuff in program visitor

* chore: check imported feature flags instead of @lwc/features import

* chore: tweaks

* chore: avoid skipping since it also skips subsequent plugins
  • Loading branch information
@ekashida
ekashida authored Aug 23, 2019
1 parent eeb6930 commit b4ac97c
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 51 deletions.
65 changes: 33 additions & 32 deletions packages/@lwc/features/src/__tests__/flags.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ const nonProdTests = {
console.log(ENABLE_FEATURE_NULL ? 'foo' : 'bar');
`,
},
'should not transform nested feature flags': {
'should transform nested feature flags': {
code: `
import { ENABLE_FEATURE_TRUE, ENABLE_FEATURE_NULL } from '@lwc/features';
if (ENABLE_FEATURE_NULL) {
Expand All @@ -121,7 +121,7 @@ const nonProdTests = {
import { ENABLE_FEATURE_TRUE, ENABLE_FEATURE_NULL, runtimeFlags } from '@lwc/features';
if (runtimeFlags.ENABLE_FEATURE_NULL) {
if (ENABLE_FEATURE_TRUE) {
if (runtimeFlags.ENABLE_FEATURE_TRUE) {
console.log('this looks like a bad idea');
}
}
Expand Down Expand Up @@ -202,6 +202,37 @@ pluginTester({
import { ENABLE_FEATURE_FALSE, runtimeFlags } from '@lwc/features';
`,
},
// Override of nonProdTest version
'should transform nested feature flags': {
code: `
import { ENABLE_FEATURE_TRUE, ENABLE_FEATURE_NULL } from '@lwc/features';
if (ENABLE_FEATURE_NULL) {
if (ENABLE_FEATURE_TRUE) {
console.log('nested feature flags sounds like a vary bad idea');
}
}
if (ENABLE_FEATURE_TRUE) {
if (ENABLE_FEATURE_NULL) {
console.log('nested feature flags sounds like a vary bad idea');
}
}
`,
output: `
import { ENABLE_FEATURE_TRUE, ENABLE_FEATURE_NULL, runtimeFlags } from '@lwc/features';
if (runtimeFlags.ENABLE_FEATURE_NULL) {
{
console.log('nested feature flags sounds like a vary bad idea');
}
}
{
if (runtimeFlags.ENABLE_FEATURE_NULL) {
console.log('nested feature flags sounds like a vary bad idea');
}
}
`,
},
'should transform both boolean and null feature flags': {
code: `
import { ENABLE_FEATURE_TRUE, ENABLE_FEATURE_FALSE, ENABLE_FEATURE_NULL } from '@lwc/features';
Expand Down Expand Up @@ -294,35 +325,5 @@ pluginTester({
console.log(runtimeFlags.ENABLE_FEATURE_NULL ? 'foo' : 'bar');
`,
},
'should not transform nested feature flags': {
code: `
import { ENABLE_FEATURE_TRUE, ENABLE_FEATURE_NULL } from '@lwc/features';
if (ENABLE_FEATURE_NULL) {
if (ENABLE_FEATURE_TRUE) {
console.log('nested feature flags sounds like a vary bad idea');
}
}
if (ENABLE_FEATURE_TRUE) {
if (ENABLE_FEATURE_NULL) {
console.log('nested feature flags sounds like a vary bad idea');
}
}
`,
output: `
import { ENABLE_FEATURE_TRUE, ENABLE_FEATURE_NULL, runtimeFlags } from '@lwc/features';
if (runtimeFlags.ENABLE_FEATURE_NULL) {
if (ENABLE_FEATURE_TRUE) {
console.log('nested feature flags sounds like a vary bad idea');
}
}
{
if (ENABLE_FEATURE_NULL) {
console.log('nested feature flags sounds like a vary bad idea');
}
}
`,
},
}),
});
41 changes: 22 additions & 19 deletions packages/@lwc/features/src/babel-plugin/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ const defaultFeatureFlags = require('../../');

const RUNTIME_FLAGS_IDENTIFIER = 'runtimeFlags';

function isRuntimeFlagLookup(path, state) {
function isRuntimeFlag(path, featureFlags) {
return (
path.isMemberExpression() &&
state.featureFlags[path.node.property.name] !== undefined &&
featureFlags[path.node.property.name] !== undefined &&
path.get('object').isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER })
);
}
Expand All @@ -20,12 +20,20 @@ module.exports = function({ types: t }) {
return {
name: 'babel-plugin-lwc-features',
visitor: {
ImportDeclaration(path, state) {
// `pre()` doesn't have access to the `this.opts` plugin options so
// we initialize in the Program visitor instead.
Program() {
this.featureFlagIfStatements = [];
this.featureFlags = this.opts.featureFlags || defaultFeatureFlags;
this.importDeclarationScope = [];
this.importedFeatureFlags = [];
},
ImportDeclaration(path) {
if (path.node.source.value === '@lwc/features') {
const specifiers = path.get('specifiers');

state.importDeclarationScope = path.scope;
state.importedFeatureFlags = specifiers
this.importDeclarationScope = path.scope;
this.importedFeatureFlags = specifiers
.map(specifier => specifier.node.imported.name)
.filter(name => name === name.toUpperCase());

Expand All @@ -45,27 +53,25 @@ module.exports = function({ types: t }) {
}
}
},
IfStatement(path, state) {
IfStatement(path) {
const testPath = path.get('test');

state.featureFlags =
state.featureFlags || state.opts.featureFlags || defaultFeatureFlags;

// If we have imported any flags and the if-test is a plain identifier
if (state.importDeclarationScope && testPath.isIdentifier()) {
if (this.importedFeatureFlags.length && testPath.isIdentifier()) {
const name = testPath.node.name;
const binding = state.importDeclarationScope.getBinding(name);
const binding = this.importDeclarationScope.getBinding(name);

// The identifier is a feature flag if it matches the name
// of an imported feature flag binding and it's a reference
// from the import declaration scope.
const isFeatureFlag =
state.importedFeatureFlags.includes(name) &&
this.importedFeatureFlags.includes(name) &&
binding &&
binding.referencePaths.includes(testPath);

if (isFeatureFlag) {
const value = state.featureFlags[name];
if (!state.opts.prod || value === null) {
const value = this.featureFlags[name];
if (!this.opts.prod || value === null) {
testPath.replaceWithSourceString(`${RUNTIME_FLAGS_IDENTIFIER}.${name}`);
// We replaced this identifier with a member
// expression that uses the same identifier and we
Expand All @@ -85,9 +91,9 @@ module.exports = function({ types: t }) {
// appropriate for production mode. This essentially undoes the
// non-production mode transform of forcing all flags to be
// runtime flags.
if (state.opts.prod && isRuntimeFlagLookup(testPath, state)) {
if (this.opts.prod && isRuntimeFlag(testPath, this.featureFlags)) {
const name = testPath.node.property.name;
const value = state.featureFlags[name];
const value = this.featureFlags[name];
if (value === true) {
// Transform the IfStatement into a BlockStatement
path.replaceWith(path.node.consequent);
Expand All @@ -97,9 +103,6 @@ module.exports = function({ types: t }) {
path.remove();
}
}

// Nested feature flags sounds like a very bad idea
path.skip();
},
},
};
Expand Down

0 comments on commit b4ac97c

Please sign in to comment.