feat: add freezeTemplate() API, warn on mutation #2825

nolanlawson · 2022-05-05T23:43:40Z

Details

This is a rewrite of #2783. It's less ambitious and more targeted.

This PR:

Does require a compiler change
Does not require updating the ACT compiler
Adds a convenient hook for freezing the template, called freezeTemplate(). Today, this function just warns, but in the future, it can fully freeze (W-10950976).

This does require exporting a new API called freezeTemplate, hence why this is a feat: rather than a fix:.

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

This is extremely unlikely to break anyone, since the observable changes are just 1) a new exported API, and 2) new console logging.

GUS work item

W-11013609

nolanlawson · 2022-05-05T23:44:58Z

packages/@lwc/compiler/src/transformers/template.ts

@@ -83,6 +84,7 @@ function serialize(
    buffer += 'if (_implicitStylesheets || _implicitScopedStylesheets) {\n';
    buffer += `  tmpl.stylesheetToken = "${scopeToken}"\n`;
    buffer += '}\n';
+    buffer += 'freezeTemplate(tmpl);\n';


This does not strictly need to be added to the ACT compiler, since all it does is log a warning. Maybe someday when this actually freezes, we can add this to the ACT compiler.

In either case, it'd be just a one-line code change to the ACT compiler.

nolanlawson · 2022-05-05T23:46:01Z

packages/@lwc/engine-core/src/framework/secure-template.ts

@@ -34,6 +34,8 @@ export function registerTemplate(tpl: Template): Template {
    // on top of stylesheetToken for anyone who is accessing the old internal API.
    // Details: https://salesforce.quip.com/v1rmAFu2cKAr
    defineProperty(tpl, 'stylesheetTokens', {
+        enumerable: true,
+        configurable: true,


The configurable allows us to define another descriptor on top of this descriptor, which helps isolate the freezing/warning logic to freezeTemplate.

The enumerable is just for consistency's sake, since the same thing happens when you do tmpl.stylesheets = [].

nolanlawson · 2022-05-05T23:46:39Z

packages/integration-karma/helpers/test-utils.js

@@ -93,23 +93,28 @@ window.TestUtils = (function (lwc, jasmine, beforeAll) {
                        },
                    };
                },
-                compare: function compare(actual, expectedMessage) {
-                    function matchMessage(message) {
+                compare: function compare(actual, expectedMessages) {


I needed a way to test multiple dev warnings, which is why I had to modify this.

nolanlawson · 2022-05-06T16:06:14Z

/nucleus ignore --reason "lwc-platform needs its tests to be updated"

divmain

LGTM

divmain · 2022-05-06T21:05:59Z

packages/@lwc/engine-core/src/framework/freeze-template.ts

+const TEMPLATE_PROPS = ['slots', 'stylesheetToken', 'stylesheets', 'renderMode'] as const;
+
+// Via https://www.npmjs.com/package/object-observer
+const ARRAY_MUTATION_METHODS = [


This bit seems like it should live in @lwc/shared. Possibly getOriginalArrayMethod also.

I kind of feel it should rather live here, since I don't think any other code will want to import it. It makes it simpler to remove when the warning is replaced with Object.freeze().

I suppose one could say the same thing about the new Array methods I added (e.g. ArrayReverse), but there was already a precedent for those things being in @lwc/shared.

Hmm... I suppose that's fair, since this is a temporary fix. 👍

packages/@lwc/engine-core/src/framework/freeze-template.ts

divmain · 2022-05-06T21:13:56Z

packages/@lwc/engine-core/src/framework/freeze-template.ts

+                // Avoid logging twice (for both stylesheetToken and stylesheetTokens)
+                mutationWarningsSilenced = true;
+                originalDescriptor!.set!.call(this, value);
+                mutationWarningsSilenced = false;


Do we want a warning to show in the console for each set, or a single warning when it is done the first time?

Good question. I considered doing the bookkeeping to ensure we only warn once (per component that calls the setter), but since I don't expect these setters to be called very often, I thought the risk of the console being flooded was low, and wasn't worth the extra code for the bookkeeping.

nolanlawson · 2022-05-06T22:05:10Z

/nucleus ignore --reason "lwc-platform needs its tests to be updated"

jmsjtu

LGTM!

nolanlawson · 2022-05-09T20:00:26Z

/nucleus test

ravijayaramappa · 2022-05-09T22:52:00Z

@nolanlawson Give a heads up to the locker team to add freezeTemplate to https://github.com/salesforce/locker/blob/master/packages/locker-core-lwc/src/SecureLWC.js

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

feat: add freezeTemplate() API, warn on mutation

879d7e3

nolanlawson requested a review from pmdartus May 5, 2022 23:43

nolanlawson commented May 5, 2022

View reviewed changes

nolanlawson mentioned this pull request May 5, 2022

feat: add registerStylesheets util, warn on template mutation #2783

Closed

nolanlawson added 2 commits May 5, 2022 16:59

fix: warn on slots/renderMode as well

8155914

fix: add comment

1113803

nolanlawson mentioned this pull request May 6, 2022

Freeze the template object #2826

Open

divmain approved these changes May 6, 2022

View reviewed changes

fix: remove duplicate process.env.NODE_ENV check

8b671a6

jmsjtu approved these changes May 6, 2022

View reviewed changes

nolanlawson merged commit 5af18fd into master May 9, 2022

nolanlawson deleted the nolan/tmpl-func-warn-2 branch May 9, 2022 21:57

nolanlawson mentioned this pull request Jun 2, 2022

test(integration-karma): update ACT components #2862

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add freezeTemplate() API, warn on mutation #2825

feat: add freezeTemplate() API, warn on mutation #2825

nolanlawson commented May 5, 2022

nolanlawson May 5, 2022 •

edited

Loading

nolanlawson May 5, 2022

nolanlawson May 5, 2022

nolanlawson commented May 6, 2022

divmain left a comment

divmain May 6, 2022

nolanlawson May 6, 2022

divmain May 6, 2022

divmain May 6, 2022

nolanlawson May 6, 2022

nolanlawson commented May 6, 2022

jmsjtu left a comment

nolanlawson commented May 9, 2022

ravijayaramappa commented May 9, 2022

feat: add freezeTemplate() API, warn on mutation #2825

feat: add freezeTemplate() API, warn on mutation #2825

Conversation

nolanlawson commented May 5, 2022

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

nolanlawson May 5, 2022 • edited Loading

Choose a reason for hiding this comment

nolanlawson May 5, 2022

Choose a reason for hiding this comment

nolanlawson May 5, 2022

Choose a reason for hiding this comment

nolanlawson commented May 6, 2022

divmain left a comment

Choose a reason for hiding this comment

divmain May 6, 2022

Choose a reason for hiding this comment

nolanlawson May 6, 2022

Choose a reason for hiding this comment

divmain May 6, 2022

Choose a reason for hiding this comment

divmain May 6, 2022

Choose a reason for hiding this comment

nolanlawson May 6, 2022

Choose a reason for hiding this comment

nolanlawson commented May 6, 2022

jmsjtu left a comment

Choose a reason for hiding this comment

nolanlawson commented May 9, 2022

ravijayaramappa commented May 9, 2022

nolanlawson May 5, 2022 •

edited

Loading