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

feat(engine-server): initial directory structure with tests #1922

Merged
merged 8 commits into from
Jun 17, 2020
Merged

feat(engine-server): initial directory structure with tests #1922

merged 8 commits into from
Jun 17, 2020

Conversation

ekashida
Copy link
Member

Details

Does this PR introduce breaking changes?

  • ✅ No, it does not introduce breaking changes.

The PR fulfills these requirements:

  • Have tests for the proposed changes been added? ✅

GUS work item

pmdartus
pmdartus reviewed Jun 12, 2020
View reviewed changes
packages/@lwc/engine-core/src/framework/restrictions.ts
@@ -91,31 +91,31 @@ function getNodeRestrictionsDescriptors(
return {
appendChild: generateDataDescriptor({
value(this: Node, aChild: Node) {
if (this instanceof Element && isFalse(options.isPortal)) {
Copy link
Member

Choose a reason for hiding this comment

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

@caridy do you recall why the Element check was needed here? It was introduced with the lwc:dom="manual" directive: bd64274#diff-8d59e8cac3f4cb87581a75c21f6d97e7R41-R44. We also don't have any specific test coverage for this either.

Copy link
Contributor

@ravijayaramappa ravijayaramappa Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Could be due to this:
The restrictions routine getNodeRestrictionsDescriptors is meant to be invoked for any Node but the portal restrictions are only meant for Elements.
Currently this method is invoked for the host, shadowRoot and all elements. Host and shadowRoot do not need these restrictions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember exactly why, but @ravijayaramappa 's assumptions sound about right. The shadowRoot/fragments are the problem here. We can chat about it.

Copy link
Member

Choose a reason for hiding this comment

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

Good point @ravijayaramappa. I created another PR to work around this issue without relying on instanceof: #1934

packages/@lwc/engine-core/src/framework/renderer.ts

// This is a temporary workaround to get the @lwc/engine-server to evaluate in node without having
// to inject at runtime.
export const documentObject: Document = typeof document !== 'undefined' ? document : ({} as any);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be fixed at some point for the @lwc/engine-server to handle AOM and global HTML attributes.

packages/@lwc/engine-core/src/framework/base-lightning-element.ts
@@ -175,12 +175,6 @@ function BaseLightningElementConstructor(this: LightningElement): LightningEleme
if (isNull(vmBeingConstructed)) {
throw new ReferenceError('Illegal constructor');
}
if (process.env.NODE_ENV !== 'production') {
Copy link
Member

Choose a reason for hiding this comment

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

This check is not needed after the spring cleaning.

Copy link
Contributor

Choose a reason for hiding this comment

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

PM, can you please elaborate how elm is guaranteed to be an HTMLElement instance?
It wasn't clear from https://github.com/salesforce/lwc/pull/1893/files how it is guaranteed.

Copy link
Member

Choose a reason for hiding this comment

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

There is no guaranty at the type level. We have struggled since the beginning of the SSR work to find the right balance with typings. We can't check in the engine if a given element is an instance of a give DOM constructor since it couple the engine with the DOM. I am open to any idea on how to work around this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could inject a validation function via the renderer or a similar mechanism?

packages/@lwc/engine-core/src/framework/stylesheet.ts Show resolved Hide resolved
jodarove
jodarove reviewed Jun 15, 2020
View reviewed changes
packages/@lwc/engine-core/src/framework/vm.ts
@@ -243,12 +243,6 @@ export function createVM<HostNode, HostElement>(
renderer: Renderer;
}
): VM {
if (process.env.NODE_ENV !== 'production') {
assert.invariant(
elm instanceof HTMLElement,
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder why we had this assert?!
looking at the spring20 branch, the type is the same for elm, so i would assume ts will catch these cases.

Copy link
Member

Choose a reason for hiding this comment

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

Good question, I don't know either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Verified that this code path is only ever hit internally so we should be able to remove it.

jodarove
jodarove reviewed Jun 15, 2020
View reviewed changes
packages/@lwc/engine-server/src/utils/elements.ts Outdated
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
const VOID_ELEMENT_NAMES = new Set([
Copy link
Contributor

@jodarove jodarove Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

why is this list different than the one used in the html parser ?

Copy link
Member

Choose a reason for hiding this comment

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

Good point, I will extract this to the @lwc/shared.

ravijayaramappa
ravijayaramappa reviewed Jun 16, 2020
View reviewed changes
packages/@lwc/engine-core/src/framework/def.ts
@@ -254,7 +254,7 @@ export function getComponentInternalDef(Ctor: unknown, name?: string): Component

// Only set prototype for public methods and properties
// No DOM Patching occurs here
export function setElementProto(elm: Element, def: ComponentDef) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to use a generic here? It is not being referenced anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

The HostElement generic is needed to remove the Element type of the elm argument.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point @ravijayaramappa. I don't think we need the generic here since it is not used in the function or in the retuned type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see what you meant!

ravijayaramappa
ravijayaramappa reviewed Jun 16, 2020
View reviewed changes
Copy link
Contributor

@ravijayaramappa ravijayaramappa left a comment

Choose a reason for hiding this comment

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

LGTM, there are some questions/nit pointed out on the review.

@pmdartus pmdartus mentioned this pull request Jun 16, 2020
Merged
jodarove
jodarove reviewed Jun 16, 2020
View reviewed changes
packages/@lwc/engine-server/package.json Outdated
"types/"
],
"devDependencies": {
"@lwc/engine-core": "1.7.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be 1.7.2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to 1.7.2 after rebasing

jodarove
jodarove approved these changes Jun 16, 2020
View reviewed changes
Copy link
Contributor

@jodarove jodarove left a comment

Choose a reason for hiding this comment

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

lgtm

@ekashida ekashida merged commit e2e361a into master Jun 17, 2020
@ekashida ekashida deleted the ekashida/engine-server branch June 17, 2020 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmdartus pmdartus pmdartus left review comments

@jodarove jodarove jodarove approved these changes

@ravijayaramappa ravijayaramappa ravijayaramappa approved these changes

@caridy caridy Awaiting requested review from caridy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ekashida @caridy @jodarove @ravijayaramappa @pmdartus