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

Show basic TypeScript errors #120

Merged
merged 8 commits into from
Mar 29, 2021
Merged

Show basic TypeScript errors #120

merged 8 commits into from
Mar 29, 2021

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Mar 28, 2021

  • Syntactic and other basic TypeScript errors are now shown with red underlines and a tooltip on hover.

  • We currently can't show any errors that depend on typings, because we don't fetch them (even the standard libs)! This will require some more work -- tracking separately at Fetch and import dependency and standard lib typings #119 and will send as followup.

  • It's using the Language Server Protocol Diagnostic type as the interface. Should be good for other diagnostic sources we might add in the future.

  • Sadly, we're currently displaying a border-bottom: 2px dashed red, instead of text-decoration: red wavy underline, because wavy doesn't render anything for single characters (!): https://bugs.chromium.org/p/chromium/issues/detail?id=668042). Could add a custom implementation of wavy underlines as a followup.

  • Try it out at https://polymerlabs.github.io/playground-elements/

Part of #67

@aomarks aomarks requested a review from justinfagnani March 28, 2021 17:34
Copy link
Collaborator

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

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

very nice!

/* It would be nice to use "text-decoration: red wavy underline" here,
but unfortunately it renders nothing at all for single characters.
See https://bugs.chromium.org/p/chromium/issues/detail?id=668042. */
border-bottom: var(--playground-error-border, 2px red dashed);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: this seems to layout fine, but I wondered if outline was the correct property to use.?

Copy link
Member Author

Choose a reason for hiding this comment

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

outline always applies to all 4 sides, not just the bottom. I think border doesn't mess with layout here because CodeMirror fixes the height of every line.

@@ -203,6 +189,9 @@ export class PlaygroundCodeEditor extends LitElement {
case 'pragmas':
this._applyHideAndFoldRegions();
break;
case 'diagnostics':
this._showDiagnostics();
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd love to have an @observe decorator or similar for this pattern...

* Convert a diagnostic from TypeScript format to Language Server Protocol
* format.
*/
function makeLspDiagnostic(tsDiagnostic: ts.Diagnostic): lsp.Diagnostic {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. You didn't see a function like this in typescript itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

@aomarks aomarks merged commit c80a58c into master Mar 29, 2021
@aomarks aomarks deleted the errors branch March 29, 2021 16:17
augustjk pushed a commit to augustjk/playground-elements that referenced this pull request Jul 18, 2022
google#111 renderItem is sometimes called with undefined
augustjk pushed a commit to augustjk/playground-elements that referenced this pull request Jul 18, 2022
augustjk pushed a commit to augustjk/playground-elements that referenced this pull request Jul 18, 2022
…irtualizer-directive

Apply fix from pull request google#120 to directive implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants