From efc21c62744c3d7375ece494ee98d66a4bffeef3 Mon Sep 17 00:00:00 2001 From: Eugene Kashida Date: Tue, 8 Oct 2019 20:29:30 +0900 Subject: [PATCH 1/6] test: add support for unary negation --- .../@lwc/features/src/__tests__/flags.spec.ts | 155 +++++++++--------- 1 file changed, 74 insertions(+), 81 deletions(-) diff --git a/packages/@lwc/features/src/__tests__/flags.spec.ts b/packages/@lwc/features/src/__tests__/flags.spec.ts index 7c97981fd0..a32e993b54 100644 --- a/packages/@lwc/features/src/__tests__/flags.spec.ts +++ b/packages/@lwc/features/src/__tests__/flags.spec.ts @@ -12,14 +12,21 @@ const nonProdTests = { code: ` import featureFlags from '@lwc/features'; if (featureFlags.ENABLE_FEATURE_TRUE) { - console.log('ENABLE_FEATURE_TRUE'); + console.log('featureFlags.ENABLE_FEATURE_TRUE'); + } + if (!featureFlags.ENABLE_FEATURE_TRUE) { + console.log('!featureFlags.ENABLE_FEATURE_TRUE'); } `, output: ` import featureFlags, { runtimeFlags } from '@lwc/features'; if (runtimeFlags.ENABLE_FEATURE_TRUE) { - console.log('ENABLE_FEATURE_TRUE'); + console.log('featureFlags.ENABLE_FEATURE_TRUE'); + } + + if (!runtimeFlags.ENABLE_FEATURE_TRUE) { + console.log('!featureFlags.ENABLE_FEATURE_TRUE'); } `, }, @@ -27,14 +34,21 @@ const nonProdTests = { code: ` import features from '@lwc/features'; if (features.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('features.ENABLE_FEATURE_FALSE'); + } + if (!features.ENABLE_FEATURE_FALSE) { + console.log('!features.ENABLE_FEATURE_FALSE'); } `, output: ` import features, { runtimeFlags } from '@lwc/features'; if (runtimeFlags.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('features.ENABLE_FEATURE_FALSE'); + } + + if (!runtimeFlags.ENABLE_FEATURE_FALSE) { + console.log('!features.ENABLE_FEATURE_FALSE'); } `, }, @@ -42,43 +56,21 @@ const nonProdTests = { code: ` import features from '@lwc/features'; if (features.ENABLE_FEATURE_NULL) { - console.log('ENABLE_FEATURE_NULL'); + console.log('features.ENABLE_FEATURE_NULL'); + } + if (!features.ENABLE_FEATURE_NULL) { + console.log('!features.ENABLE_FEATURE_NULL'); } `, output: ` import features, { runtimeFlags } from '@lwc/features'; if (runtimeFlags.ENABLE_FEATURE_NULL) { - console.log('ENABLE_FEATURE_NULL'); - } - `, - }, - 'should not transform feature flags unless the if-test is a simple member expression': { - code: ` - import FEATURES from '@lwc/features'; - if (FEATURES.ENABLE_FEATURE_NULL === null) { - console.log('ENABLE_FEATURE_NULL === null'); - } - if (isTrue(FEATURES.ENABLE_FEATURE_TRUE)) { - console.log('isTrue(ENABLE_FEATURE_TRUE)'); - } - if (!FEATURES.ENABLE_FEATURE_FALSE) { - console.log('!ENABLE_FEATURE_FALSE'); - } - `, - output: ` - import FEATURES, { runtimeFlags } from '@lwc/features'; - - if (FEATURES.ENABLE_FEATURE_NULL === null) { - console.log('ENABLE_FEATURE_NULL === null'); - } - - if (isTrue(FEATURES.ENABLE_FEATURE_TRUE)) { - console.log('isTrue(ENABLE_FEATURE_TRUE)'); + console.log('features.ENABLE_FEATURE_NULL'); } - if (!FEATURES.ENABLE_FEATURE_FALSE) { - console.log('!ENABLE_FEATURE_FALSE'); + if (!runtimeFlags.ENABLE_FEATURE_NULL) { + console.log('!features.ENABLE_FEATURE_NULL'); } `, }, @@ -86,12 +78,15 @@ const nonProdTests = { code: ` import featureFlag from '@lwc/features'; if (featureFlag.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('featureFlag.ENABLE_FEATURE_FALSE'); } function awesome() { const featureFlag = { ENABLE_FEATURE_FALSE: false }; if (featureFlag.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('featureFlag.ENABLE_FEATURE_FALSE'); + } + if (!featureFlag.ENABLE_FEATURE_FALSE) { + console.log('!featureFlag.ENABLE_FEATURE_FALSE'); } } `, @@ -99,7 +94,7 @@ const nonProdTests = { import featureFlag, { runtimeFlags } from '@lwc/features'; if (runtimeFlags.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('featureFlag.ENABLE_FEATURE_FALSE'); } function awesome() { @@ -108,7 +103,11 @@ const nonProdTests = { }; if (featureFlag.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('featureFlag.ENABLE_FEATURE_FALSE'); + } + + if (!featureFlag.ENABLE_FEATURE_FALSE) { + console.log('!featureFlag.ENABLE_FEATURE_FALSE'); } } `, @@ -130,6 +129,9 @@ const nonProdTests = { if (featureFlags.ENABLE_FEATURE_TRUE) { console.log('this looks like a bad idea'); } + if (!featureFlags.ENABLE_FEATURE_TRUE) { + console.log('this looks like a bad idea'); + } } `, output: ` @@ -139,6 +141,10 @@ const nonProdTests = { if (runtimeFlags.ENABLE_FEATURE_TRUE) { console.log('this looks like a bad idea'); } + + if (!runtimeFlags.ENABLE_FEATURE_TRUE) { + console.log('this looks like a bad idea'); + } } `, }, @@ -173,13 +179,16 @@ const nonProdTestOverrides = { code: ` import features from '@lwc/features'; if (features.ENABLE_FEATURE_TRUE) { - console.log('ENABLE_FEATURE_TRUE'); + console.log('features.ENABLE_FEATURE_TRUE'); + } + if (!features.ENABLE_FEATURE_TRUE) { + console.log('!features.ENABLE_FEATURE_TRUE'); } `, output: ` import features, { runtimeFlags } from '@lwc/features'; { - console.log('ENABLE_FEATURE_TRUE'); + console.log('features.ENABLE_FEATURE_TRUE'); } `, }, @@ -187,11 +196,17 @@ const nonProdTestOverrides = { code: ` import features from '@lwc/features'; if (features.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('features.ENABLE_FEATURE_FALSE'); + } + if (!features.ENABLE_FEATURE_FALSE) { + console.log('!features.ENABLE_FEATURE_FALSE'); } `, output: ` import features, { runtimeFlags } from '@lwc/features'; + { + console.log('!features.ENABLE_FEATURE_FALSE'); + } `, }, 'should transform nested feature flags': { @@ -233,7 +248,10 @@ const nonProdTestOverrides = { function awesome() { const featureFlag = { ENABLE_FEATURE_FALSE: false }; if (featureFlag.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('featureFlag.ENABLE_FEATURE_FALSE'); + } + if (!featureFlag.ENABLE_FEATURE_FALSE) { + console.log('!featureFlag.ENABLE_FEATURE_FALSE'); } } `, @@ -246,7 +264,11 @@ const nonProdTestOverrides = { }; if (featureFlag.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('featureFlag.ENABLE_FEATURE_FALSE'); + } + + if (!featureFlag.ENABLE_FEATURE_FALSE) { + console.log('!featureFlag.ENABLE_FEATURE_FALSE'); } } `, @@ -266,28 +288,29 @@ pluginTester({ code: ` import features from '@lwc/features'; if (features.ENABLE_FEATURE_TRUE) { - console.log('ENABLE_FEATURE_TRUE'); + console.log('features.ENABLE_FEATURE_TRUE'); } if (features.ENABLE_FEATURE_FALSE) { - console.log('ENABLE_FEATURE_FALSE'); + console.log('features.ENABLE_FEATURE_FALSE'); } if (features.ENABLE_FEATURE_NULL) { - console.log('ENABLE_FEATURE_NULL'); + console.log('features.ENABLE_FEATURE_NULL'); } `, output: ` import features, { runtimeFlags } from '@lwc/features'; { - console.log('ENABLE_FEATURE_TRUE'); + console.log('features.ENABLE_FEATURE_TRUE'); } if (runtimeFlags.ENABLE_FEATURE_NULL) { - console.log('ENABLE_FEATURE_NULL'); + console.log('features.ENABLE_FEATURE_NULL'); } `, }, 'should transform runtime flag lookups into compile-time flags': { code: ` + import { runtimeFlags } from '@lwc/features'; if (runtimeFlags.ENABLE_FEATURE_TRUE) { console.log('runtimeFlags.ENABLE_FEATURE_TRUE'); } @@ -299,6 +322,7 @@ pluginTester({ } `, output: ` + import { runtimeFlags } from '@lwc/features'; { console.log('runtimeFlags.ENABLE_FEATURE_TRUE'); } @@ -308,51 +332,20 @@ pluginTester({ } `, }, - 'should not transform runtime flag lookups unless the if-test is a member expression': { - code: ` - if (runtimeFlags.ENABLE_FEATURE_NULL === null) { - console.log('runtimeFlags.ENABLE_FEATURE_NULL === null'); - } - if (isTrue(runtimeFlags.ENABLE_FEATURE_TRUE)) { - console.log('runtimeFlags.ENABLE_FEATURE_TRUE'); - } - if (!runtimeFlags.ENABLE_FEATURE_FALSE) { - console.log('!runtimeFlags.ENABLE_FEATURE_FALSE'); - } - `, - output: ` - if (runtimeFlags.ENABLE_FEATURE_NULL === null) { - console.log('runtimeFlags.ENABLE_FEATURE_NULL === null'); - } - - if (isTrue(runtimeFlags.ENABLE_FEATURE_TRUE)) { - console.log('runtimeFlags.ENABLE_FEATURE_TRUE'); - } - - if (!runtimeFlags.ENABLE_FEATURE_FALSE) { - console.log('!runtimeFlags.ENABLE_FEATURE_FALSE'); - } - `, - }, 'should not transform member expressions that are not runtime flag lookups': { code: ` + import { runtimeFlags } from '@lwc/features'; if (churroteria.ENABLE_FEATURE_TRUE) { console.log('churroteria.ENABLE_FEATURE_TRUE'); } `, output: ` + import { runtimeFlags } from '@lwc/features'; + if (churroteria.ENABLE_FEATURE_TRUE) { console.log('churroteria.ENABLE_FEATURE_TRUE'); } `, }, - 'should not transform runtime flags when used with a ternary operator': { - code: ` - console.log(runtimeFlags.ENABLE_FEATURE_NULL ? 'foo' : 'bar'); - `, - output: ` - console.log(runtimeFlags.ENABLE_FEATURE_NULL ? 'foo' : 'bar'); - `, - }, }), }); From 27aa119a122ca4344ea7e30565194026f0209a16 Mon Sep 17 00:00:00 2001 From: Eugene Kashida Date: Tue, 8 Oct 2019 20:31:03 +0900 Subject: [PATCH 2/6] refactor(features): add support for unary negation Now also throws an exception when: - Feature flags are used incorrectly (but only within an if-statement test) - Runtime flags are used directly in non-prod modes --- .../@lwc/features/src/babel-plugin/index.js | 176 ++++++++++-------- 1 file changed, 95 insertions(+), 81 deletions(-) diff --git a/packages/@lwc/features/src/babel-plugin/index.js b/packages/@lwc/features/src/babel-plugin/index.js index f25ed5dd01..a72c3986ed 100644 --- a/packages/@lwc/features/src/babel-plugin/index.js +++ b/packages/@lwc/features/src/babel-plugin/index.js @@ -8,24 +8,9 @@ const defaultFeatureFlags = require('../../').default; const RUNTIME_FLAGS_IDENTIFIER = 'runtimeFlags'; -function isRuntimeFlag(path, featureFlags) { - return ( - path.isMemberExpression() && - path.get('object').isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }) && - path.get('property').isIdentifier() && - featureFlags[path.node.property.name] !== undefined - ); -} - -function validate(name, value) { - if (!/[A-Z_]+/.test(name)) { - throw new Error( - `Invalid feature flag "${name}". Flag name must only be composed of uppercase letters and underscores.` - ); - } - if (value === undefined) { - throw new Error(`Invalid feature flag "${name}". Flag is undefined.`); - } +function isBindingReference(path, scope) { + const binding = scope && scope.getBinding(path.node.name); + return !!(binding && binding.referencePaths.includes(path)); } module.exports = function({ types: t }) { @@ -35,27 +20,29 @@ module.exports = function({ types: t }) { // `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 = []; }, - ImportDefaultSpecifier(defaultSpecifierPath) { - const importDeclarationPath = defaultSpecifierPath.findParent(p => - p.isImportDeclaration() - ); - if (importDeclarationPath.node.source.value === '@lwc/features') { - this.importDeclarationScope = importDeclarationPath.scope; - this.defaultSpecifierName = defaultSpecifierPath.node.local.name; - const specifiers = importDeclarationPath.get('specifiers'); + ImportDeclaration(path) { + if (path.node.source.value === '@lwc/features') { + this.importDeclarationPath = path; + } + }, + ImportDefaultSpecifier(path) { + // If this is the default specifier for the @lwc/features import declaration + if (path.parentPath === this.importDeclarationPath) { + this.defaultImportPath = path; + const specifiers = this.importDeclarationPath.node.specifiers; const didImportRuntimeFlags = specifiers - .filter(specifier => specifier !== defaultSpecifierPath) + // Filter out the default import specifier + .filter(specifier => specifier !== path.node) + // Check if we've already imported runtime flags .some(specifier => { - return specifier.node.imported.name === RUNTIME_FLAGS_IDENTIFIER; + return specifier.imported.name === RUNTIME_FLAGS_IDENTIFIER; }); if (!didImportRuntimeFlags) { // Blindly import a binding for `runtimeFlags`. Tree-shaking // will simply remove it if unused. - importDeclarationPath.node.specifiers.push( + this.importDeclarationPath.node.specifiers.push( t.importSpecifier( t.identifier(RUNTIME_FLAGS_IDENTIFIER), t.identifier(RUNTIME_FLAGS_IDENTIFIER) @@ -65,57 +52,84 @@ module.exports = function({ types: t }) { } }, IfStatement(path) { - const testPath = path.get('test'); - - // If we have imported the feature flags lookup (default binding) and the if-test is a member expression. - if (this.defaultSpecifierName && testPath.isMemberExpression()) { - const objectPath = testPath.get('object'); - const propertyPath = testPath.get('property'); - // If the member expression is a shallow feature flag lookup (i.e., the property is an identifier). - if ( - objectPath.isIdentifier({ name: this.defaultSpecifierName }) && - propertyPath.isIdentifier() - ) { - const binding = this.importDeclarationScope.getBinding( - objectPath.node.name - ); - // If this thing is an actual reference to the imported feature flag lookup. - if (binding && binding.referencePaths.includes(objectPath)) { - const name = propertyPath.node.name; - const value = this.featureFlags[name]; - validate(name, value); - if (!this.opts.prod || value === null) { - testPath.replaceWithSourceString( - `${RUNTIME_FLAGS_IDENTIFIER}.${name}` - ); - } else if (value === true) { - // Transform the IfStatement into a BlockStatement - path.replaceWith(path.node.consequent); - } else if (value === false) { - // Remove IfStatement - path.remove(); - } - } - } - } - - // Transform runtime flags into compile-time flags, where appropriate, for - // production mode. This serves to undo the non-production mode transform of - // forcing all flags to be runtime flags. - if (this.opts.prod && isRuntimeFlag(testPath, this.featureFlags)) { - const name = testPath.node.property.name; - const value = this.featureFlags[name]; - validate(name, value); - if (value === true) { - // Transform the IfStatement into a BlockStatement - path.replaceWith(path.node.consequent); - } - if (value === false) { - // Remove IfStatement - path.remove(); - } - } + path.traverse(FeatureFlagVisitor, this); }, }, }; }; + +const FeatureFlagVisitor = { + MemberExpression(path) { + const objectPath = path.get('object'); + const propertyPath = path.get('property'); + const isCompileTimeLookup = + this.defaultImportPath && + objectPath.isIdentifier({ name: this.defaultImportPath.node.local.name }); + const isRuntimeLookup = objectPath.isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }); + const importDeclarationScope = + this.importDeclarationPath && this.importDeclarationPath.scope; + if ( + (isCompileTimeLookup || isRuntimeLookup) && + propertyPath.isIdentifier() && + isBindingReference(objectPath, importDeclarationScope) + ) { + validate.call(this, path); + + const isUnaryNegation = path.parentPath.isUnaryExpression({ operator: '!' }); + const ifStatementPath = isUnaryNegation ? path.parentPath.parentPath : path.parentPath; + + const name = propertyPath.node.name; + let value = this.featureFlags[name]; + + if (!this.opts.prod || value === null) { + path.replaceWithSourceString(`${RUNTIME_FLAGS_IDENTIFIER}.${name}`); + return; + } + + if (this.opts.prod) { + if (isUnaryNegation) { + value = !value; + } + if (value === true) { + // Transform the IfStatement into a BlockStatement + ifStatementPath.replaceWith(ifStatementPath.node.consequent); + } + if (value === false) { + // Remove IfStatement + ifStatementPath.remove(); + } + } + } + }, +}; + +function validate(memberExpressionPath) { + const name = memberExpressionPath.node.property.name; + const value = this.featureFlags[name]; + if (!/[A-Z_]+/.test(name)) { + throw new Error( + `Invalid feature flag "${name}". Flag name must only be composed of uppercase letters and underscores.` + ); + } + if (value === undefined) { + throw new Error(`Invalid feature flag "${name}". Flag is undefined.`); + } + + let parentPath = memberExpressionPath.parentPath; + if (parentPath.isUnaryExpression({ operator: '!' })) { + parentPath = parentPath.parentPath; + } + if (!parentPath.isIfStatement()) { + throw new Error( + `Member expressions or unary negations of member expressions are the only supported ways to use feature flags.` + ); + } + + const objectPath = memberExpressionPath.get('object'); + const isRuntimeLookup = objectPath.isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }); + if (isRuntimeLookup && !this.opts.prod) { + throw new Error( + `Runtime flags should never be used directly and should only be added by the compiler.` + ); + } +} From c46d72f4fa52fef82f94f37f0541acb6d836b25b Mon Sep 17 00:00:00 2001 From: Eugene Kashida Date: Tue, 8 Oct 2019 21:07:34 +0900 Subject: [PATCH 3/6] test: add coverage for exceptions --- .../@lwc/features/src/__tests__/flags.spec.ts | 49 +++++++++++++++++-- .../@lwc/features/src/babel-plugin/index.js | 2 +- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/packages/@lwc/features/src/__tests__/flags.spec.ts b/packages/@lwc/features/src/__tests__/flags.spec.ts index a32e993b54..af6aa61e42 100644 --- a/packages/@lwc/features/src/__tests__/flags.spec.ts +++ b/packages/@lwc/features/src/__tests__/flags.spec.ts @@ -148,13 +148,40 @@ const nonProdTests = { } `, }, + 'should throw an error if the flag is undefined': { + error: 'Invalid feature flag "ENABLE_THE_BEER". Flag is undefined.', + code: ` + import featureFlags from '@lwc/features'; + if (featureFlags.ENABLE_THE_BEER) { + console.log('featureFlags.ENABLE_THE_BEER'); + } + `, + }, + 'should throw an error if the flag name is formatted incorrectly': { + error: + 'Invalid feature flag "enable_the_beer". Flag name must only be composed of uppercase letters and underscores.', + code: ` + import featureFlags from '@lwc/features'; + if (featureFlags.enable_the_beer) { + console.log('featureFlags.enable_the_beer'); + } + `, + }, + 'should throw an error if the test is not a member expression': { + error: `Member expressions or unary negations of member expressions are the only supported ways to use feature flags.`, + code: ` + import featureFlags from '@lwc/features'; + if (featureFlags.ENABLE_FEATURE_TRUE === true) { + console.log('featureFlags.ENABLE_FEATURE_TRUE === true'); + } + `, + }, }; const featureFlags = { ENABLE_FEATURE_TRUE: true, ENABLE_FEATURE_FALSE: false, ENABLE_FEATURE_NULL: null, - invalidFeatureFlag: true, // invalid because it's not all uppercase }; const babelOptions = { @@ -169,7 +196,19 @@ pluginTester({ featureFlags, }, babelOptions, - tests: nonProdTests, + tests: { + ...nonProdTests, + 'should throw an error if runtime flags are manually introduced': { + error: + 'Runtime flags should never be used directly and should only be added by the compiler.', + code: ` + import { runtimeFlags } from '@lwc/features'; + if (runtimeFlags.ENABLE_FEATURE_TRUE) { + console.log('runtimeFlags.ENABLE_FEATURE_TRUE'); + } + `, + }, + }, }); // These tests override corresponding tests in nonProdTests since the plugin has @@ -283,7 +322,9 @@ pluginTester({ prod: true, }, babelOptions, - tests: Object.assign({}, nonProdTests, nonProdTestOverrides, { + tests: { + ...nonProdTests, + ...nonProdTestOverrides, 'should transform both boolean and null feature flags': { code: ` import features from '@lwc/features'; @@ -347,5 +388,5 @@ pluginTester({ } `, }, - }), + }, }); diff --git a/packages/@lwc/features/src/babel-plugin/index.js b/packages/@lwc/features/src/babel-plugin/index.js index a72c3986ed..ba020e567b 100644 --- a/packages/@lwc/features/src/babel-plugin/index.js +++ b/packages/@lwc/features/src/babel-plugin/index.js @@ -106,7 +106,7 @@ const FeatureFlagVisitor = { function validate(memberExpressionPath) { const name = memberExpressionPath.node.property.name; const value = this.featureFlags[name]; - if (!/[A-Z_]+/.test(name)) { + if (!/^[A-Z_]+$/.test(name)) { throw new Error( `Invalid feature flag "${name}". Flag name must only be composed of uppercase letters and underscores.` ); From 9c6c59abb74d010dec846b0a8eaee1403484a2b6 Mon Sep 17 00:00:00 2001 From: Eugene Kashida Date: Thu, 10 Oct 2019 05:05:33 +0900 Subject: [PATCH 4/6] refactor: visitor optimization --- .../@lwc/features/src/__tests__/flags.spec.ts | 53 +++++-- .../@lwc/features/src/babel-plugin/index.js | 140 +++++++++--------- 2 files changed, 113 insertions(+), 80 deletions(-) diff --git a/packages/@lwc/features/src/__tests__/flags.spec.ts b/packages/@lwc/features/src/__tests__/flags.spec.ts index af6aa61e42..c4dc91f6ed 100644 --- a/packages/@lwc/features/src/__tests__/flags.spec.ts +++ b/packages/@lwc/features/src/__tests__/flags.spec.ts @@ -74,6 +74,28 @@ const nonProdTests = { } `, }, + 'should not transform tests that are not a member expression or not a negated member expression (compile time)': { + code: ` + import FEATURES from '@lwc/features'; + if (FEATURES.ENABLE_FEATURE_NULL === null) { + console.log('ENABLE_FEATURE_NULL === null'); + } + if (isTrue(FEATURES.ENABLE_FEATURE_TRUE)) { + console.log('isTrue(ENABLE_FEATURE_TRUE)'); + } + `, + output: ` + import FEATURES, { runtimeFlags } from '@lwc/features'; + + if (FEATURES.ENABLE_FEATURE_NULL === null) { + console.log('ENABLE_FEATURE_NULL === null'); + } + + if (isTrue(FEATURES.ENABLE_FEATURE_TRUE)) { + console.log('isTrue(ENABLE_FEATURE_TRUE)'); + } + `, + }, 'should not transform tests that are not an actual reference to the imported binding': { code: ` import featureFlag from '@lwc/features'; @@ -167,15 +189,6 @@ const nonProdTests = { } `, }, - 'should throw an error if the test is not a member expression': { - error: `Member expressions or unary negations of member expressions are the only supported ways to use feature flags.`, - code: ` - import featureFlags from '@lwc/features'; - if (featureFlags.ENABLE_FEATURE_TRUE === true) { - console.log('featureFlags.ENABLE_FEATURE_TRUE === true'); - } - `, - }, }; const featureFlags = { @@ -373,6 +386,28 @@ pluginTester({ } `, }, + 'should not transform tests that are not a member expression or not a negated member expression (runtime)': { + code: ` + import { runtimeFlags } from '@lwc/features'; + if (runtimeFlags.ENABLE_FEATURE_NULL === null) { + console.log('runtimeFlags.ENABLE_FEATURE_NULL === null'); + } + if (isTrue(runtimeFlags.ENABLE_FEATURE_TRUE)) { + console.log('runtimeFlags.ENABLE_FEATURE_TRUE'); + } + `, + output: ` + import { runtimeFlags } from '@lwc/features'; + + if (runtimeFlags.ENABLE_FEATURE_NULL === null) { + console.log('runtimeFlags.ENABLE_FEATURE_NULL === null'); + } + + if (isTrue(runtimeFlags.ENABLE_FEATURE_TRUE)) { + console.log('runtimeFlags.ENABLE_FEATURE_TRUE'); + } + `, + }, 'should not transform member expressions that are not runtime flag lookups': { code: ` import { runtimeFlags } from '@lwc/features'; diff --git a/packages/@lwc/features/src/babel-plugin/index.js b/packages/@lwc/features/src/babel-plugin/index.js index ba020e567b..2b98ae917b 100644 --- a/packages/@lwc/features/src/babel-plugin/index.js +++ b/packages/@lwc/features/src/babel-plugin/index.js @@ -8,6 +8,27 @@ const defaultFeatureFlags = require('../../').default; const RUNTIME_FLAGS_IDENTIFIER = 'runtimeFlags'; +function validate(memberExpressionPath, { featureFlags, opts }) { + const name = memberExpressionPath.node.property.name; + const value = featureFlags[name]; + if (!/^[A-Z_]+$/.test(name)) { + throw new Error( + `Invalid feature flag "${name}". Flag name must only be composed of uppercase letters and underscores.` + ); + } + if (value === undefined) { + throw new Error(`Invalid feature flag "${name}". Flag is undefined.`); + } + + const objectPath = memberExpressionPath.get('object'); + const isRuntimeLookup = objectPath.isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }); + if (isRuntimeLookup && !opts.prod) { + throw new Error( + `Runtime flags should never be used directly and should only be added by the compiler.` + ); + } +} + function isBindingReference(path, scope) { const binding = scope && scope.getBinding(path.node.name); return !!(binding && binding.referencePaths.includes(path)); @@ -52,84 +73,61 @@ module.exports = function({ types: t }) { } }, IfStatement(path) { - path.traverse(FeatureFlagVisitor, this); - }, - }, - }; -}; - -const FeatureFlagVisitor = { - MemberExpression(path) { - const objectPath = path.get('object'); - const propertyPath = path.get('property'); - const isCompileTimeLookup = - this.defaultImportPath && - objectPath.isIdentifier({ name: this.defaultImportPath.node.local.name }); - const isRuntimeLookup = objectPath.isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }); - const importDeclarationScope = - this.importDeclarationPath && this.importDeclarationPath.scope; - if ( - (isCompileTimeLookup || isRuntimeLookup) && - propertyPath.isIdentifier() && - isBindingReference(objectPath, importDeclarationScope) - ) { - validate.call(this, path); + let testPath = path.get('test'); - const isUnaryNegation = path.parentPath.isUnaryExpression({ operator: '!' }); - const ifStatementPath = isUnaryNegation ? path.parentPath.parentPath : path.parentPath; + const isUnaryNegation = testPath.isUnaryExpression({ operator: '!' }); + if (isUnaryNegation) { + testPath = testPath.get('argument'); + } - const name = propertyPath.node.name; - let value = this.featureFlags[name]; + if (!testPath.isMemberExpression()) { + return; + } - if (!this.opts.prod || value === null) { - path.replaceWithSourceString(`${RUNTIME_FLAGS_IDENTIFIER}.${name}`); - return; - } + const objectPath = testPath.get('object'); + const propertyPath = testPath.get('property'); + const isRuntimeFlag = objectPath.isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }); - if (this.opts.prod) { - if (isUnaryNegation) { - value = !value; + let isCompileTimeFlag = false; + if (this.defaultImportPath) { + isCompileTimeFlag = objectPath.isIdentifier({ + name: this.defaultImportPath.node.local.name, + }); } - if (value === true) { - // Transform the IfStatement into a BlockStatement - ifStatementPath.replaceWith(ifStatementPath.node.consequent); + + // If the member expression is not a feature flag lookup + if (!isRuntimeFlag && !isCompileTimeFlag) { + return; } - if (value === false) { - // Remove IfStatement - ifStatementPath.remove(); + // If the member expression object is not binding reference to the feature flag object + if (!isBindingReference(objectPath, this.importDeclarationPath.scope)) { + return; } - } - } - }, -}; -function validate(memberExpressionPath) { - const name = memberExpressionPath.node.property.name; - const value = this.featureFlags[name]; - if (!/^[A-Z_]+$/.test(name)) { - throw new Error( - `Invalid feature flag "${name}". Flag name must only be composed of uppercase letters and underscores.` - ); - } - if (value === undefined) { - throw new Error(`Invalid feature flag "${name}". Flag is undefined.`); - } + validate(testPath, this); - let parentPath = memberExpressionPath.parentPath; - if (parentPath.isUnaryExpression({ operator: '!' })) { - parentPath = parentPath.parentPath; - } - if (!parentPath.isIfStatement()) { - throw new Error( - `Member expressions or unary negations of member expressions are the only supported ways to use feature flags.` - ); - } + const name = propertyPath.node.name; + let value = this.featureFlags[name]; - const objectPath = memberExpressionPath.get('object'); - const isRuntimeLookup = objectPath.isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }); - if (isRuntimeLookup && !this.opts.prod) { - throw new Error( - `Runtime flags should never be used directly and should only be added by the compiler.` - ); - } -} + if (!this.opts.prod || value === null) { + testPath.replaceWithSourceString(`${RUNTIME_FLAGS_IDENTIFIER}.${name}`); + return; + } + + if (this.opts.prod) { + if (isUnaryNegation) { + value = !value; + } + if (value === true) { + // Transform the IfStatement into a BlockStatement + path.replaceWith(path.node.consequent); + } + if (value === false) { + // Remove IfStatement + path.remove(); + } + } + }, + }, + }; +}; From 3551dc5d93a6eb01a089122eb39f392180eec333 Mon Sep 17 00:00:00 2001 From: Eugene Kashida Date: Thu, 10 Oct 2019 15:57:36 +0900 Subject: [PATCH 5/6] fix: check binding ref only when import declaration exists --- .../@lwc/features/src/__tests__/flags.spec.ts | 14 +--- .../@lwc/features/src/babel-plugin/index.js | 64 ++++++++----------- 2 files changed, 29 insertions(+), 49 deletions(-) diff --git a/packages/@lwc/features/src/__tests__/flags.spec.ts b/packages/@lwc/features/src/__tests__/flags.spec.ts index c4dc91f6ed..5160a144b5 100644 --- a/packages/@lwc/features/src/__tests__/flags.spec.ts +++ b/packages/@lwc/features/src/__tests__/flags.spec.ts @@ -209,19 +209,7 @@ pluginTester({ featureFlags, }, babelOptions, - tests: { - ...nonProdTests, - 'should throw an error if runtime flags are manually introduced': { - error: - 'Runtime flags should never be used directly and should only be added by the compiler.', - code: ` - import { runtimeFlags } from '@lwc/features'; - if (runtimeFlags.ENABLE_FEATURE_TRUE) { - console.log('runtimeFlags.ENABLE_FEATURE_TRUE'); - } - `, - }, - }, + tests: nonProdTests, }); // These tests override corresponding tests in nonProdTests since the plugin has diff --git a/packages/@lwc/features/src/babel-plugin/index.js b/packages/@lwc/features/src/babel-plugin/index.js index 2b98ae917b..e6e0c56936 100644 --- a/packages/@lwc/features/src/babel-plugin/index.js +++ b/packages/@lwc/features/src/babel-plugin/index.js @@ -8,9 +8,7 @@ const defaultFeatureFlags = require('../../').default; const RUNTIME_FLAGS_IDENTIFIER = 'runtimeFlags'; -function validate(memberExpressionPath, { featureFlags, opts }) { - const name = memberExpressionPath.node.property.name; - const value = featureFlags[name]; +function validate(name, value) { if (!/^[A-Z_]+$/.test(name)) { throw new Error( `Invalid feature flag "${name}". Flag name must only be composed of uppercase letters and underscores.` @@ -19,14 +17,6 @@ function validate(memberExpressionPath, { featureFlags, opts }) { if (value === undefined) { throw new Error(`Invalid feature flag "${name}". Flag is undefined.`); } - - const objectPath = memberExpressionPath.get('object'); - const isRuntimeLookup = objectPath.isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }); - if (isRuntimeLookup && !opts.prod) { - throw new Error( - `Runtime flags should never be used directly and should only be added by the compiler.` - ); - } } function isBindingReference(path, scope) { @@ -46,24 +36,16 @@ module.exports = function({ types: t }) { ImportDeclaration(path) { if (path.node.source.value === '@lwc/features') { this.importDeclarationPath = path; - } - }, - ImportDefaultSpecifier(path) { - // If this is the default specifier for the @lwc/features import declaration - if (path.parentPath === this.importDeclarationPath) { - this.defaultImportPath = path; - const specifiers = this.importDeclarationPath.node.specifiers; - const didImportRuntimeFlags = specifiers - // Filter out the default import specifier - .filter(specifier => specifier !== path.node) - // Check if we've already imported runtime flags - .some(specifier => { - return specifier.imported.name === RUNTIME_FLAGS_IDENTIFIER; - }); + const specifiers = path.node.specifiers; + + // Check if we've already imported runtime flags + const didImportRuntimeFlags = specifiers.some(specifier => { + return specifier.local && specifier.local.name === RUNTIME_FLAGS_IDENTIFIER; + }); if (!didImportRuntimeFlags) { - // Blindly import a binding for `runtimeFlags`. Tree-shaking - // will simply remove it if unused. - this.importDeclarationPath.node.specifiers.push( + // Blindly import a binding for `runtimeFlags` if we haven't + // already. Tree-shaking will simply remove it if unused. + specifiers.push( t.importSpecifier( t.identifier(RUNTIME_FLAGS_IDENTIFIER), t.identifier(RUNTIME_FLAGS_IDENTIFIER) @@ -72,6 +54,12 @@ module.exports = function({ types: t }) { } } }, + ImportDefaultSpecifier(path) { + // If this is the default specifier for the @lwc/features import declaration + if (path.parentPath === this.importDeclarationPath) { + this.defaultImportName = path.get('local.name').node; + } + }, IfStatement(path) { let testPath = path.get('test'); @@ -89,9 +77,9 @@ module.exports = function({ types: t }) { const isRuntimeFlag = objectPath.isIdentifier({ name: RUNTIME_FLAGS_IDENTIFIER }); let isCompileTimeFlag = false; - if (this.defaultImportPath) { + if (this.defaultImportName) { isCompileTimeFlag = objectPath.isIdentifier({ - name: this.defaultImportPath.node.local.name, + name: this.defaultImportName, }); } @@ -99,19 +87,23 @@ module.exports = function({ types: t }) { if (!isRuntimeFlag && !isCompileTimeFlag) { return; } - // If the member expression object is not binding reference to the feature flag object - if (!isBindingReference(objectPath, this.importDeclarationPath.scope)) { + // If the member expression object is not a binding reference to the feature flag object + if ( + this.importDeclarationPath && + !isBindingReference(objectPath, this.importDeclarationPath.scope) + ) { return; } - validate(testPath, this); - const name = propertyPath.node.name; let value = this.featureFlags[name]; + validate(name, value); if (!this.opts.prod || value === null) { - testPath.replaceWithSourceString(`${RUNTIME_FLAGS_IDENTIFIER}.${name}`); - return; + if (isCompileTimeFlag) { + testPath.node.object = t.identifier(RUNTIME_FLAGS_IDENTIFIER); + return; + } } if (this.opts.prod) { From d32cd38e1eb028f275f0a0fe83f09cf4d5311031 Mon Sep 17 00:00:00 2001 From: Eugene Kashida Date: Thu, 10 Oct 2019 16:10:23 +0900 Subject: [PATCH 6/6] chore: address feedback --- packages/@lwc/features/src/babel-plugin/index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/@lwc/features/src/babel-plugin/index.js b/packages/@lwc/features/src/babel-plugin/index.js index e6e0c56936..77266cde24 100644 --- a/packages/@lwc/features/src/babel-plugin/index.js +++ b/packages/@lwc/features/src/babel-plugin/index.js @@ -7,6 +7,7 @@ const defaultFeatureFlags = require('../../').default; const RUNTIME_FLAGS_IDENTIFIER = 'runtimeFlags'; +const FEATURES_PACKAGE_NAME = '@lwc/features'; function validate(name, value) { if (!/^[A-Z_]+$/.test(name)) { @@ -34,7 +35,7 @@ module.exports = function({ types: t }) { this.featureFlags = this.opts.featureFlags || defaultFeatureFlags; }, ImportDeclaration(path) { - if (path.node.source.value === '@lwc/features') { + if (path.node.source.value === FEATURES_PACKAGE_NAME) { this.importDeclarationPath = path; const specifiers = path.node.specifiers; @@ -83,7 +84,7 @@ module.exports = function({ types: t }) { }); } - // If the member expression is not a feature flag lookup + // If the member expression object is neither the imported default binding nor the runtimeFlags binding if (!isRuntimeFlag && !isCompileTimeFlag) { return; }