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

feat: transition to @lwc/metadata for LWC JS introspection #570

Merged

Conversation

divmain
Copy link
Contributor

@divmain divmain commented Jun 23, 2023

What does this PR do?

This PR removes the lwc-language-server dependency on an ancient version of the LWC compiler. Now, @lwc/metadata is leveraged to provide intelligence on LWC JavaScript in the IDE.

Initially, the plan was to entirely swap out the data structures underlying a lot of the logic in the language server. Instead of relying on the very old Metadata type, all logic would be updated to instead rely on new metadata types exposed by @lwc/metadata. However, the changes necessary to make this clean swap proved to be extremely invasive. Continuing down that path would have required a de facto rewrite, with all the inevitable subtle incompatibilities and bugs that come with such an endeavor.

As an alternative, we chose to provide a mapping of @lwc/metadata to the old metadata data structure, including all of its idiosyncrasies. This approach comes with some benefits:

  • the pre-existing behavior is reproduced ~exactly (or as close as we could come to it)
  • further iterations and refactors can leverage new LWC metadata without disrupting existing functionality
  • existing tests could be leveraged, adding confidence that this PR includes only non-breaking changes
  • it should allow lwc-language-server to stay up-to-date with future releases of LWC, with the possibility that small changes to the mapping might be rarely required

As a follow-up I'll work to configure our CI to run your test suite whenever we update @lwc/metadata. If successful, the LWC team will be alerted whenever we make breaking changes to your code. In such an event, we would either 1) avoid the break or 2) proactively inform you of the change.

What issues does this PR fix or reference?

@W-13045347@

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @divmain to sign the Salesforce Inc. Contributor License Agreement.

@divmain divmain marked this pull request as draft June 23, 2023 22:37
@divmain divmain force-pushed the divmain/lwc-dependency-updates branch from 6774ac6 to 3430ef1 Compare June 26, 2023 05:08
@divmain divmain changed the title DRAFT: Transition to using @lwc/metadata for LWC JavaScript introspection feat: transition to @lwc/metadata for LWC JS introspection Jun 26, 2023
package.json Show resolved Hide resolved
Copy link
Contributor Author

@divmain divmain Jun 26, 2023

Choose a reason for hiding this comment

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

This monorepo has a top-level yarn.lock that conflicted with this package-local package-lock.json. Since the sibling packages do not have their own package-lock.json, I discerned that its existence here was probably unintentional.

@divmain divmain force-pushed the divmain/lwc-dependency-updates branch from 9b89ccb to fc924be Compare June 26, 2023 05:32
@divmain divmain force-pushed the divmain/lwc-dependency-updates branch from fc924be to cca6784 Compare June 26, 2023 05:34
@divmain divmain force-pushed the divmain/lwc-dependency-updates branch from cca6784 to 044b8fb Compare June 26, 2023 05:43
const metadata = transformerResult.metadata as Metadata;
patchComments(metadata);
return { metadata, diagnostics: [] };
transformSync(source, fileName, transformOptions);
} catch (err) {
Copy link
Contributor Author

@divmain divmain Jun 26, 2023

Choose a reason for hiding this comment

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

Some compilation errors will only appear upon an attempted compilation. These are generally restricted to non-syntax errors, such as the fact that boolean public properties cannot be true by default (otherwise, how would the parent component disable the flag?).

So, we transform first using the up-to-date compiler to ensure no compilation issues exist. If it succeeds, we collect metadata about the component in the code below.

@divmain divmain marked this pull request as ready for review June 26, 2023 07:21
@divmain divmain force-pushed the divmain/lwc-dependency-updates branch from 540d394 to 58fbbc3 Compare June 26, 2023 16:32
Copy link
Contributor

@CristiCanizales CristiCanizales left a comment

Choose a reason for hiding this comment

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

This looks good to me 🙂 And I know all the hard work to get to this 🥳
I also tested the usual features were working as expected, like:

  • go to definition (html) ✅
  • go to definition (javascript) ✅
  • autocompletion ✅
  • info on hover ✅

So I would say 🚢

packages/lwc-language-server/src/lwc-server.ts Outdated Show resolved Hide resolved
divmain and others added 2 commits July 5, 2023 17:21
Co-authored-by: Cristina Cañizales <113132642+CristiCanizales@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants