-
Notifications
You must be signed in to change notification settings - Fork 397
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(ssr): compiler error location #5114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR needs cleaning up, but I wanted to get some feedback on the approach
loc: true, | ||
source: filename, | ||
ranges: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple possible changes to avoid bloating AST:
- It didn't look like
ranges
are used by the LSP/VSCode extension, I only included it in order to matchbabel-plugin-compiler
. - Source/filename could be moved to the ComponentMetaState. That would make things a bit coupled, but it might be worth it and is what
babel-plugin-compiler
seems to do:
lwc/packages/@lwc/babel-plugin-component/src/utils.ts
Lines 101 to 113 in cf1a1b2
function generateError( source: NodePath<types.Node>, { errorInfo, messageArgs }: DecoratorErrorOptions, state: LwcBabelPluginPass ) { const message = generateErrorMessage(errorInfo, messageArgs); const error = source.buildCodeFrameError(message); (error as any).filename = state.filename; (error as any).loc = normalizeLocation(source); (error as any).lwcCode = errorInfo && errorInfo.code; return error; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching babel-plugin-component
exactly here is not necessary IMO, unless it's fulfilling some function in VS Code... and I take it it's not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least here it's not used: https://github.com/forcedotcom/lightning-language-server/blob/46ec16e555ff23dfb90a3daea060d70a31830720/packages/lwc-language-server/src/javascript/compiler.ts#L117
That's the only place I looked though.
/nucleus test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, I have no complaints, but I'd like for @wjhsf to take a look too. Thanks a bunch!
Thanks for the review @nolanlawson ! Is nucleus failing due to something unrelated? I'm not able to see the logs. |
Nucleus failures just seem like a hiccup. |
Details
Partially addresses #5032
I borrowed some tests from
babel-plugin-component
to make sure the location matches.Since it uses
generateCompilerError
from@lwc/errors
, it should work with the LSP/VSCode extension. (relevant code).TODO
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item