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.

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

perf: optimize scoped ID for light/native #4399

Merged
merged 4 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/karma.yml
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ jobs:
- run: API_VERSION=61 yarn sauce:ci
- run: API_VERSION=61 DISABLE_SYNTHETIC=1 yarn sauce:ci
- run: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 DISABLE_SYNTHETIC=1 yarn sauce:ci
- run: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 DISABLE_SYNTHETIC=1 DISABLE_STATIC_CONTENT_OPTIMIZATION=1 yarn sauce:ci
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just another case I think is worth testing, especially if we start actually shipping disableSyntheticShadowSupport=true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eagerly anticipating another chore(ci): rebalance karma tests

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 guessed it! It's imbalanced again. I wish GH had an easier way to do this. 🫠

- run: ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL=1 yarn sauce:ci
- run: ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL=1 DISABLE_SYNTHETIC=1 yarn sauce:ci
- run: DISABLE_STATIC_CONTENT_OPTIMIZATION=1 yarn sauce:ci
Expand Down
2 changes: 1 addition & 1 deletion packages/@lwc/compiler/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const { code } = transformSync(source, filename, options);
- `customRendererConfig` (type: `object`, optional) - custom renderer config to pass to `@lwc/template-compiler`. See that package's README for details.
- `enableLightningWebSecurityTransforms` (type: `boolean`, default: `false`) - The configuration to enable Lighting Web Security specific transformations.
- `enableLwcSpread` (boolean, optional, `true` by default) - Deprecated. Ignored by compiler. `lwc:spread` is always enabled.
- `disableSyntheticShadowSupport` (type: `boolean`, default: `false`) - Set to true if synthetic shadow DOM support is not needed, which can result in smaller output.
- `disableSyntheticShadowSupport` (type: `boolean`, default: `false`) - Set to true if synthetic shadow DOM support is not needed, which can result in smaller/faster output.
- `instrumentation` (type: `InstrumentationObject`, optional) - instrumentation object to gather metrics and non-error logs for internal use. See the `@lwc/errors` package for details on the interface.
- `apiVersion` (type: `number`, optional) - API version to associate with the compiled module.

Expand Down
2 changes: 1 addition & 1 deletion packages/@lwc/compiler/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export interface TransformOptions {
customRendererConfig?: CustomRendererConfig;
/** @deprecated Ignored by compiler. `lwc:spread` is always enabled. */
enableLwcSpread?: boolean;
/** Set to true if synthetic shadow DOM support is not needed, which can result in smaller output. */
/** Set to true if synthetic shadow DOM support is not needed, which can result in smaller/faster output. */
disableSyntheticShadowSupport?: boolean;
/**
* Enable transformations specific to {@link https://developer.salesforce.com/docs/platform/lwc/guide/security-lwsec-intro.html Lighting Web Security}.
Expand Down
2 changes: 2 additions & 0 deletions packages/@lwc/compiler/src/transformers/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export default function templateTransform(
namespace,
name,
apiVersion,
disableSyntheticShadowSupport,
} = options;
const experimentalDynamicDirective =
deprecatedDynamicDirective ?? Boolean(experimentalDynamicComponent);
Expand All @@ -62,6 +63,7 @@ export default function templateTransform(
enableDynamicComponents,
instrumentation,
apiVersion,
disableSyntheticShadowSupport,
});
} catch (e) {
throw normalizeToCompilerError(TransformerErrors.HTML_TRANSFORMER_ERROR, e, { filename });
Expand Down
14 changes: 0 additions & 14 deletions packages/@lwc/engine-core/src/framework/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -564,12 +564,6 @@ function k(compilerKey: number, obj: any): string | void {
function gid(id: string | undefined | null): string | null | undefined {
const vmBeingRendered = getVMBeingRendered()!;
if (isUndefined(id) || id === '') {
if (process.env.NODE_ENV !== 'production') {
logError(
`Invalid id value "${id}". The id attribute must contain a non-empty string.`,
vmBeingRendered
);
}
return id;
}
// We remove attributes when they are assigned a value of null
Expand All @@ -587,14 +581,6 @@ function gid(id: string | undefined | null): string | null | undefined {
function fid(url: string | undefined | null): string | null | undefined {
const vmBeingRendered = getVMBeingRendered()!;
if (isUndefined(url) || url === '') {
if (process.env.NODE_ENV !== 'production') {
if (isUndefined(url)) {
logError(
`Undefined url value for "href" or "xlink:href" attribute. Expected a non-empty string.`,
vmBeingRendered
);
}
}
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 got rid of these logErrors because they seemed pretty low-value (only logged in dev mode, should arguably be the job of accessibility tools like aXe, not LWC), and I couldn't think of a good way to maintain the dev-only logging when the optimization is in place.

return url;
}
// We remove attributes when they are assigned a value of null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,15 @@ describe('scoped-ids', () => {
describe('custom elements', () => {
it('should render expected id attribute value when its value is set to `undefined`', () => {
const elm = createElement('x-foo', { is: CustomElementIdValueUndefined });
expect(() => {
document.body.appendChild(elm);
}).toLogErrorDev(
/\[LWC error\]: Invalid id value "undefined". The id attribute must contain a non-empty string./
);
document.body.appendChild(elm);
const child = elm.shadowRoot.querySelector('x-child');
// #1769: The difference in behavior of id attribute is tracked with this issue
expect(child.getAttribute('id')).toBe('undefined');
});

it('should render the id attribute as a boolean attribute when its value is set to an empty string', () => {
const elm = createElement('x-foo', { is: CustomElementIdValueEmpty });
expect(() => {
document.body.appendChild(elm);
}).toLogErrorDev(
/\[LWC error\]: Invalid id value "". The id attribute must contain a non-empty string./
);
document.body.appendChild(elm);
const child = elm.shadowRoot.querySelector('x-child');
expect(child.getAttribute('id')).toBe('');
});
Expand All @@ -44,22 +36,14 @@ describe('scoped-ids', () => {
describe('native elements', () => {
it('should not render id attribute when its value is set to null', () => {
const elm = createElement('x-foo', { is: IdValueUndefined });
expect(() => {
document.body.appendChild(elm);
}).toLogErrorDev(
/\[LWC error\]: Invalid id value "undefined". The id attribute must contain a non-empty string./
);
document.body.appendChild(elm);
const div = elm.shadowRoot.querySelector('div');
expect(div.getAttribute('id')).toBeNull();
});

it('should render the id attribute as a boolean attribute when its value is set to an empty string', () => {
const elm = createElement('x-foo', { is: IdValueEmpty });
expect(() => {
document.body.appendChild(elm);
}).toLogErrorDev(
/\[LWC error\]: Invalid id value "". The id attribute must contain a non-empty string./
);
document.body.appendChild(elm);
const div = elm.shadowRoot.querySelector('div');
expect(div.getAttribute('id')).toBe('');
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { createElement } from 'lwc';
import { spyConsole } from 'test-utils';

import AriaStatic from 'x/ariaStatic';
import AriaDynamic from 'x/ariaDynamic';
Expand All @@ -26,27 +25,8 @@ function testAria(type, create) {
beforeEach(async () => {
elm = create();

const consoleSpy = spyConsole();
try {
document.body.appendChild(elm);
await Promise.resolve();

// empty string (`<div id="">`) is expected to log an error, but not boolean true (`<div id>`)
// due to backwards compat
if (process.env.NODE_ENV !== 'production' && type === 'empty-string') {
expect(consoleSpy.calls.error.length).toEqual(10);
for (const call of consoleSpy.calls.error) {
expect(call[0].message).toMatch(
/The id attribute must contain a non-empty string/
);
}
} else {
expect(consoleSpy.calls.error.length).toEqual(0);
}
expect(consoleSpy.calls.warn.length).toEqual(0);
} finally {
consoleSpy.reset();
}
document.body.appendChild(elm);
await Promise.resolve();
});

it('should transform the `for` attribute value', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import HrefDynamic from 'x/hrefDynamic';
import HrefDangling from 'x/hrefDangling';
import HrefBooleanTrue from 'x/hrefBooleanTrue';
import HrefBooleanTrueNoId from 'x/hrefBooleanTrueNoId';
import HrefDynamicEmptyString from 'x/hrefDynamicEmptyString';
import HrefDynamicUndefined from 'x/hrefDynamicUndefined';
import HrefDynamicNull from 'x/hrefDynamicNull';

function testHref(type, create) {
describe(`${type} href attribute values`, () => {
Expand Down Expand Up @@ -96,3 +99,30 @@ describe('boolean true', () => {
});
});
});

describe('dynamic empty value', () => {
const scenarios = [
{
name: 'empty string',
Ctor: HrefDynamicEmptyString,
tagName: 'x-href-dynamic-empty-string',
},
{ name: 'undefined', Ctor: HrefDynamicUndefined, tagName: 'x-href-dynamic-undefined' },
{ name: 'null', Ctor: HrefDynamicNull, tagName: 'x-href-dynamic-null' },
];

scenarios.forEach(({ name, Ctor, tagName }) => {
describe(name, () => {
it('renders href as expected', () => {
const elm = createElement(tagName, { is: Ctor });
document.body.appendChild(elm);

const expected = name === 'empty string' ? '' : null;

expect(elm.shadowRoot.querySelector('.sanjo').getAttribute('id')).toBe(expected);
expect(elm.shadowRoot.querySelector('a').getAttribute('href')).toBe(expected);
expect(elm.shadowRoot.querySelector('area').getAttribute('href')).toBe(expected);
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<template>
<div class="sanjo" id={emptyString}></div>

<div>
<a class="anchor-empty-string" href={emptyString} data-id={emptyString}>fragment url</a>
</div>

<div>
<map name="blackdot">
<area class="area-empty-string" href={emptyString} data-id={emptyString} shape="circle" coords="75,75,75" />
</map>
</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { LightningElement } from 'lwc';

export default class HrefDynamicEmptyString extends LightningElement {
get emptyString() {
return '';
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<template>
<div class="sanjo" id={nullo}></div>

<div>
<a class="anchor-empty-string" href={nullo} data-id={nullo}>fragment url</a>
</div>

<div>
<map name="blackdot">
<area class="area-empty-string" href={nullo} data-id={nullo} shape="circle" coords="75,75,75" />
</map>
</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { LightningElement } from 'lwc';

export default class HrefDynamicNull extends LightningElement {
get nullo() {
return null;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<template>
<div class="sanjo" id={undef}></div>

<div>
<a class="anchor-empty-string" href={undef} data-id={undef}>fragment url</a>
</div>

<div>
<map name="blackdot">
<area class="area-empty-string" href={undef} data-id={undef} shape="circle" coords="75,75,75" />
</map>
</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { LightningElement } from 'lwc';

export default class HrefDynamicUndefined extends LightningElement {
get undef() {
return undefined;
}
}
1 change: 1 addition & 0 deletions packages/@lwc/template-compiler/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ const { code, warnings } = compile(`<template><h1>Hello World!</h1></template>`,
- `enableLwcSpread` (boolean, optional, `true` by default) - Deprecated. Ignored by template-compiler. `lwc:spread` is always enabled.
- `customRendererConfig` (CustomRendererConfig, optional) - specifies a configuration to use to match elements. Matched elements will get a custom renderer hook in the generated template.
- `instrumentation` (InstrumentationObject, optional) - instrumentation object to gather metrics and non-error logs for internal use. See the `@lwc/errors` package for details on the interface.
- `disableSyntheticShadowSupport` (type: `boolean`, default: `false`) - Set to true if synthetic shadow DOM support is not needed, which can result in smaller/faster output.

- Example 1: Config to match `<use>` elements under the `svg` namespace and have `href` attribute set.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ describe('customRendererConfig normalization', () => {
},
],
},
"disableSyntheticShadowSupport": false,
"enableDynamicComponents": false,
"enableLwcSpread": true,
"enableStaticContentOptimization": true,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<template lwc:render-mode="light">
<!-- static values -->
<label for="foo">Click me:</label>
<input type="checkbox" id="foo">

<a href="#bar">Click to scroll</a>
<section id="bar">Scroll to me</section>

<!-- dynamic values -->
<label for={foo}>Click me:</label>
<input type="checkbox" id={foo}>

<a href={bar}>Click to scroll</a>
<section id={bar}>Scroll to me</section>
</template>
Loading
Loading