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

nolanlawson · 2022-04-29T19:21:23Z

Details

Fixes #2811. Adds a test to confirm that feature flags work in @lwc/engine-dom, @lwc/engine-core, and @lwc/synthetic-shadow. (I didn't see flags being used in any other packages, although in the future we could add more tests if we want.)

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

✅ No, it does not introduce an observable change.

GUS work item

W-11072441

Fixes #2811

nolanlawson · 2022-04-29T19:22:06Z

scripts/rollup/lwcFeatures.js

+                id.includes('/node_modules/') ||
+                id.includes('/dist/') ||
+                !source.includes('@lwc/features')
+            ) {


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.

nolanlawson · 2022-04-29T19:23:10Z

packages/@lwc/engine-core/src/testFeatureFlag.ts

+            })
+        );
+    });
+}


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.

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, ... }

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

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.

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.

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

This reverts commit 72afdc0.

ravijayaramappa · 2022-04-29T23:16:52Z

packages/@lwc/engine-core/src/testFeatureFlag.ts

+import features from '@lwc/features';
+
+if (process.env.NODE_ENV !== 'production') {
+    if (features.ENABLE_TEST_EXCEPTION) {


How is this test designed to work?

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

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.

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

@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.

Makes sense. Thanks for clarifying.

ravijayaramappa · 2022-04-29T23:20:10Z

packages/@lwc/engine-dom/scripts/rollup.config.js

@@ -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');


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

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

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.

pmdartus · 2022-05-02T07:32:32Z

I still don't fully understand this PR, but something looks wrong in the way we approach feature flags in LWC. From the outside, it feels that we are adding a lot of complexity.

nolanlawson · 2022-05-02T15:58:19Z

I still don't fully understand this PR, but something looks wrong in the way we approach feature flags in LWC. From the outside, it feels that we are adding a lot of complexity.

I agree, I am just trying to work within the existing system. As-is, feature flags just do not work in @lwc/engine-dom.

nolanlawson · 2022-05-02T21:49:53Z

Based on feedback for this PR and confusion about what exactly we're testing, I've restored the tests I originally wrote. These tests really do verify that feature flags are 100% working, and the new Karma tests will fail if anything is messed up.

The new tests are not very pretty, but they accurately reflect whether feature flags are working or not.

wip: add a static vnode type wip: needs key and patch wip: all tests passing exept a few because of simple things wip: set style tokens for fragments during template evaluation wip: set element shadow token for fragments during template evaluation wip: propagate the shadow resolver key of static fragments wip: do not gen static nodes for text or comments wip: use tagedTemplate expression to replate stylesheetToken wip: use cloneNode from renderer wip: treeWalker to work in ie11 refactor: do not strip empty attr or empty class attr fix: using incorrect key wip: trim value of textNodes and review feedback fix: hydration feat: custom static element serializer wip: remove unessesary import fix: hydration fix: snapshot tests fix: missing karma test fix: test due rebase test: add test for static content needing nodeOwner fix: escape strings in serializer refactor: remove unused apis on generated code refactor: review suggestions fix: support mixed mode wip: fix compilation snapshots fix: increase 0.5kb bundlesize for engine dom fix: flapper wip: helpers.ts review wip: codegen.ts review wip: missing items from pm review wip: review comments fix: respect preserveComments and fuse $1,2 into 3 fix: svg content with the correct namespace feat(template-compiler): add option to disable static content optimizations wip: remove invalid comment chore: bump version to v2.13.0 (#2784) chore: dependencies upgrade (#2785) test: fix Node warning about event emitters (#2789) chore: run karma and integration tests in parallel (#2792) * chore: run karma and integration tests in parallel * fix: remove log lines fix(babel-plugin-component): remove import validation (#2719) test: remove flakey IE integration test (#2796) test: update test to use lwc imports (#2794) chore: Restrict further import order (#2795) chore: bump version to v2.13.1 (#2804) refactor(engine): moving vm references from dom into core (#2801) * refactor(engine): moving vm references from dom into core chore(nucleus): remove salesforcedevs/developer-website (#2807) test(integration-karma): small quality-of-life improvements (#2809) chore(deps): bump ejs from 3.1.6 to 3.1.7 (#2810) chore: weekly dependencies upgrade (#2816) * chore: weekly dependencies upgrade * fix: update yarn.lock` refactor(engine): optimize computation of transitive shadow mode (#2803) chore(deps): bump async from 2.6.3 to 2.6.4 (#2815) Bumps [async](https://github.com/caolan/async) from 2.6.3 to 2.6.4. - [Release notes](https://github.com/caolan/async/releases) - [Changelog](https://github.com/caolan/async/blob/v2.6.4/CHANGELOG.md) - [Commits](caolan/async@v2.6.3...v2.6.4) --- updated-dependencies: - dependency-name: async dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore: bump version to v2.13.2 (#2819) chore: retry failed Circle CI tests (#2814) * chore: retry failed Circle CI tests W-10946477 * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: deliberately fail a test to see what happens * chore: improve retry script * fix: whitespace * Revert "chore: deliberately fail a test to see what happens" This reverts commit 611fc34. * chore: rename to retry_karma fix(engine-core): add shim for old stylesheetTokens internal API (#2821) W-11093934 chore: bump version to v2.13.3 (#2823) fix(build): remove swc, switch back to babel and terser (#2818) feat: add freezeTemplate() API, warn on mutation (#2825) * feat: add freezeTemplate() API, warn on mutation * fix: warn on slots/renderMode as well * fix: add comment * fix: remove duplicate process.env.NODE_ENV check fix(engine-dom): refactor stylesheet API for consistency (#2827) * fix(engine-dom): refactor stylesheet API for consistency * fix: remove useless code comment * test: remove unnecessary test * test: remove unnecessary test * refactor: slight refactor * fix: add code comments * fix: add code comments * fix: add better comment fix: relax static id validation in iterations (#2830) fix(rollup-plugin): emit warnings during compilation (#2833) * fix(rollup-plugin): emit warnings during compilation Fixes #2771 W-10930894 * fix: add code comment fix(engine-dom): make feature flags work (#2812) * fix(engine-dom): make feature flags work Fixes #2811 * fix: license headers * test: fix jest tests * test: fix test * test: fix test * fix: use Eugene's technique instead * Revert "fix: use Eugene's technique instead" This reverts commit 72afdc0. * fix: use Eugene's technique instead * fix: revert unnecessary test change * fix: revert, use the elaborate test instead * fix: fix feature flags in engine-server as well perf(engine-dom): refactor style cache to reduce lookups (#2832) * perf(engine-dom): refactor style cache to reduce lookups * fix: tidy up comments * fix: update packages/@lwc/engine-dom/src/styles.ts Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> * fix: remove semi * fix: remove "used" flag * fix: refactor * fix: refactor * fix: bring back "used" flag * fix: typo Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> chore: update deps (#2838) test: run feature flag test code only in karma (#2835) fix: trigger slotchange event on removing slot (#2840) test(integration-karma): silence lwc rollup plugin warnings (#2836) * test(integration-karma): silence lwc rollup plugin warnings * fix: use warn API v2.11.7 (#2842) chore: release v2.14.0 (#2846) fix: only remove slot children in synthetic shadow (#2843) * fix: only remove slot children in synthetic shadow * fix: use case block fix: only add version mismatch test code in karma (#2852) test(integration-karma): ensure constructable stylesheets are re-used (#2844) * test(integration-karma): ensure constructable stylesheets are re-used * test: add test for shared style chore(nucleus): remove more downstreams (#2855) chore(nucleus): remove another downstream (#2857) docs: fix typo in template compiler readme (#2848) * docs: fix typo in template compiler readme * docs: rewording usage of lwc dynamic directive Co-authored-by: Eugene Kashida <ekashida@gmail.com> Co-authored-by: Eugene Kashida <ekashida@gmail.com> chore: fix lint test: refactor test, remove test covered in #2859 test: on second thought, bring test back

fix(engine-dom): make feature flags work

3799eb9

Fixes #2811

nolanlawson requested a review from ekashida April 29, 2022 19:21

nolanlawson commented Apr 29, 2022

View reviewed changes

nolanlawson added 8 commits April 29, 2022 12:24

fix: license headers

9f6afe1

test: fix jest tests

43efa19

test: fix test

e2b74c9

test: fix test

76c66b6

fix: use Eugene's technique instead

72afdc0

Revert "fix: use Eugene's technique instead"

79a41c8

This reverts commit 72afdc0.

fix: use Eugene's technique instead

8c08a6e

fix: revert unnecessary test change

01c9664

ravijayaramappa reviewed Apr 29, 2022

View reviewed changes

nolanlawson added 2 commits May 2, 2022 14:40

fix: revert, use the elaborate test instead

a18f086

fix: fix feature flags in engine-server as well

61f1eb7

ravijayaramappa approved these changes May 16, 2022

View reviewed changes

nolanlawson merged commit d9bdaa5 into master May 16, 2022

nolanlawson deleted the nolan/issue-2811 branch May 16, 2022 19:56

nolanlawson mentioned this pull request May 16, 2022

test: run feature flag test code only in karma #2835

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nolanlawson commented Apr 29, 2022

nolanlawson Apr 29, 2022

nolanlawson Apr 29, 2022

ekashida Apr 29, 2022

nolanlawson Apr 29, 2022

nolanlawson Apr 29, 2022

nolanlawson Apr 29, 2022

nolanlawson May 2, 2022

ravijayaramappa Apr 29, 2022 •

edited

Loading

jmsjtu Apr 30, 2022

nolanlawson May 2, 2022

ravijayaramappa May 2, 2022

nolanlawson May 2, 2022 •

edited

Loading

ravijayaramappa May 3, 2022

ravijayaramappa Apr 29, 2022

nolanlawson May 2, 2022

nolanlawson May 2, 2022

pmdartus commented May 2, 2022

nolanlawson commented May 2, 2022

nolanlawson commented May 2, 2022

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

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

Conversation

nolanlawson commented Apr 29, 2022

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravijayaramappa Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmdartus commented May 2, 2022

nolanlawson commented May 2, 2022

nolanlawson commented May 2, 2022

ravijayaramappa Apr 29, 2022 •

edited

Loading

nolanlawson May 2, 2022 •

edited

Loading