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.

Sign up for GitHub

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

Jump to bottom

fix(engine-dom): make feature flags work #2812

Merged
merged 11 commits into from
May 16, 2022
Merged

fix(engine-dom): make feature flags work #2812

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3799eb9
fix(engine-dom): make feature flags work
nolanlawson Apr 29, 2022
9f6afe1
fix: license headers
nolanlawson Apr 29, 2022
43efa19
test: fix jest tests
nolanlawson Apr 29, 2022
e2b74c9
test: fix test
nolanlawson Apr 29, 2022
76c66b6
test: fix test
nolanlawson Apr 29, 2022
72afdc0
fix: use Eugene's technique instead
nolanlawson Apr 29, 2022
79a41c8
Revert "fix: use Eugene's technique instead"
nolanlawson Apr 29, 2022
8c08a6e
fix: use Eugene's technique instead
nolanlawson Apr 29, 2022
01c9664
fix: revert unnecessary test change
nolanlawson Apr 29, 2022
a18f086
fix: revert, use the elaborate test instead
nolanlawson May 2, 2022
61f1eb7
fix: fix feature flags in engine-server as well
nolanlawson May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/@lwc/engine-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,7 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

// Tests
import './testFeatureFlag';

export * from './framework/main';
14 changes: 14 additions & 0 deletions packages/@lwc/engine-core/src/testFeatureFlag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright (c) 2018, salesforce.com, inc.
* All rights reserved.
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import features from '@lwc/features';

if (process.env.NODE_ENV !== 'production') {
if (features.ENABLE_TEST_EXCEPTION) {
Copy link
Contributor

@ravijayaramappa ravijayaramappa Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

How is this test designed to work?

Copy link
Member

Choose a reason for hiding this comment

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

I also had a question about this, when would features.ENABLE_TEST_EXCEPTION be set to true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's never set to true. The point is just that, if the compilation pipeline is broken, then this will throw an error. It was previously throwing an error because @lwc/engine-dom's Rollup scripts were not configured properly.

I can add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

The point is just that, if the compilation pipeline is broken, then this will throw an error.

Was this throwing the error at runtime?

I tried to insert these statements and run yarn build locally and it didn't throw. Test Branch: https://github.com/salesforce/lwc/tree/ravi/test-feature-flag

Copy link
Collaborator Author

@nolanlawson nolanlawson May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ravijayaramappa In your branch, the feature flags don't actually work. 🙂 If you try doing setFeatureFlagForTest then nothing will happen, because the LWC features Babel plugin is not being applied to the code.

Whereas if you do apply the Babel plugin, it breaks because it runs twice on both @lwc/engine-dom and @lwc/engine-core... that's what this PR is fixing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Thanks for clarifying.

throw new Error('Compile-time processing of feature flags is broken.');
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It'd be great if we didn't have to ship this code even in dev mode, but I can't think of a good way to add code that only matters for our Karma tests.

I could also abstract this into a shared utility in @lwc/shared, but it seems silly since, again, it's only used in tests.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding a compilation-time feature flag that is forever configured to false?

For example:

// @lwc/engine-dom/src/index.ts
...
import features from '@lwc/features';
if (features.ENABLE_TEST_EXCEPTION) {
  throw new Error('Compile-time processing of feature flags is broken.');
}
...

// @lwc/features/flags.ts
const features: FeatureFlagMap = {
    ...
    ENABLE_TEST_EXCEPTION: false,
    ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think this is a much simpler solution than mine. Let's do it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah wait, except what this doesn't cover is if the features Babel plugin doesn't run at all. In that case, the flag will just silently not work, and no error will be thrown.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You know what, maybe that's fine, if a feature flag is not working in @lwc/engine-dom, then presumably the test for that flag will fail anyway. Let's go with your solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted due to some confusion about what the tests were doing.

2 changes: 2 additions & 0 deletions packages/@lwc/engine-dom/scripts/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const path = require('path');
const { nodeResolve } = require('@rollup/plugin-node-resolve');
const typescript = require('../../../../scripts/rollup/typescript');
const writeDistAndTypes = require('../../../../scripts/rollup/writeDistAndTypes');
const lwcFeatures = require('../../../../scripts/rollup/lwcFeatures');
Copy link
Contributor

Choose a reason for hiding this comment

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

Need the same change in packages/@lwc/engine-server/scripts/rollup.config.js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't bother to make this change for engine-server. I guess I should go ahead and do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I could make the change to engine-server, but then the problem is that there's no way to test it. The karma tests don't test engine-server, and the unit tests use the src TypeScript files rather than the dist files (after running them through Rollup).

I'll just make the change without a test.

const { version } = require('../package.json');

const banner = `/* proxy-compat-disable */`;
Expand All @@ -35,6 +36,7 @@ module.exports = {
}),
typescript(),
writeDistAndTypes(),
lwcFeatures(),
],

onwarn({ code, message }) {
Expand Down
3 changes: 3 additions & 0 deletions packages/@lwc/engine-dom/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

// Tests -------------------------------------------------------------------------------------------
import './testFeatureFlag.ts';

// Polyfills ---------------------------------------------------------------------------------------
import './polyfills/aria-properties/main';

Expand Down
14 changes: 14 additions & 0 deletions packages/@lwc/engine-dom/src/testFeatureFlag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright (c) 2018, salesforce.com, inc.
* All rights reserved.
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import features from '@lwc/features';

if (process.env.NODE_ENV !== 'production') {
if (features.ENABLE_TEST_EXCEPTION) {
throw new Error('Compile-time processing of feature flags is broken.');
}
}
1 change: 1 addition & 0 deletions packages/@lwc/features/src/flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const features: FeatureFlagMap = {
ENABLE_NODE_PATCH: null,
ENABLE_REACTIVE_SETTER: null,
ENABLE_WIRE_SYNC_EMIT: null,
ENABLE_TEST_EXCEPTION: null,
};

if (!globalThis.lwcRuntimeFlags) {
Expand Down
6 changes: 6 additions & 0 deletions packages/@lwc/features/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ export interface FeatureFlagMap {
* of next tick. It only affects wire configurations that depend on component values.
*/
ENABLE_WIRE_SYNC_EMIT: FeatureFlagValue;

/**
* This is only used to test that feature flags are actually working. If set to true, it
* should throw an error.
*/
ENABLE_TEST_EXCEPTION: FeatureFlagValue;
}

export type FeatureFlagName = keyof FeatureFlagMap;
3 changes: 3 additions & 0 deletions packages/@lwc/synthetic-shadow/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

// Tests
import './testFeatureFlag';

// Collecting env references before patching anything
import './env/node';
import './env/element';
Expand Down
14 changes: 14 additions & 0 deletions packages/@lwc/synthetic-shadow/src/testFeatureFlag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright (c) 2018, salesforce.com, inc.
* All rights reserved.
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import features from '@lwc/features';

if (process.env.NODE_ENV !== 'production') {
if (features.ENABLE_TEST_EXCEPTION) {
throw new Error('Compile-time processing of feature flags is broken.');
}
}
7 changes: 6 additions & 1 deletion scripts/rollup/lwcFeatures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,13 @@ module.exports = function lwcFeatures() {
return {
id: 'rollup-plugin-lwc-features',
transform(source, id) {
if (id.includes('/node_modules/') || !source.includes('@lwc/features')) {
if (
id.includes('/node_modules/') ||
id.includes('/dist/') ||
!source.includes('@lwc/features')
) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the key to making the feature flags work. @lwc/engine-dom was pulling in the dist file from @lwc/engine-core, which had already been processed by the features Babel plugin. So the features Babel plugin was complaining that only the default export should be used.

// Skip 3rd-party files and files that don't mention @lwc/features
// Also skip /dist/ files because these have presumably already been run through lwcFeatures
return null;
}
return babel.transform(source, {
Expand Down