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(website): Show tsconfig parsing errors in tab #10991

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

developer-bandi
Copy link
Contributor

@developer-bandi developer-bandi commented Mar 23, 2025

PR Checklist

Overview

I'm trying to manage tsconfig errors with different markers and statuses.

Because I couldn't integrate tsconfig errors into markers since markers change depending on editor changes, and I thought it was wrong to apply tsconfig where markers are created.

treat in below comment, change marker type to Record<TabType,ErrorGroup[]>

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @developer-bandi!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@developer-bandi developer-bandi marked this pull request as draft March 23, 2025 15:00
Copy link

netlify bot commented Mar 23, 2025

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit f8903df
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/67e55b68cf05590008a05d1a
😎 Deploy Preview https://deploy-preview-10991--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 91 (🔴 down 8 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

nx-cloud bot commented Mar 23, 2025

View your CI Pipeline Execution ↗ for commit f8903df.

Command Status Duration Result
nx run-many --target=build --exclude website --... ✅ Succeeded 1s View ↗
nx run-many --target=clean ✅ Succeeded 10s View ↗

☁️ Nx Cloud last updated this comment at 2025-03-31 12:46:25 UTC

@armano2
Copy link
Collaborator

armano2 commented Mar 23, 2025

maybe we should just extends fs to contain info about errors for specific files, with that we could change/swap what we display based on file that is selected?

tsconfig is a file (model) in monaco, similar to any other file, and its cloned as vfs file

@developer-bandi
Copy link
Contributor Author

developer-bandi commented Mar 24, 2025

maybe we should just extends fs to contain info about errors for specific files, with that we could change/swap what we display based on file that is selected?

tsconfig is a file (model) in monaco, similar to any other file, and its cloned as vfs file

If I understand correctly, you mean changing the type of markers to Record<TabType,ErrorGroup[]> and displaying the corresponding error depending on the selected tab?

@developer-bandi developer-bandi marked this pull request as ready for review March 27, 2025 14:02
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started! I think I found a bug? Also left a question and a few refactor suggestions.

console.error(e);
if (e instanceof Error) {
const error = {
group: 'Typescript',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
group: 'Typescript',
group: 'TypeScript',

prev.tsconfig.map(error => [error.group, error]),
);

activeTabErrors.Typescript = error;
Copy link
Member

Choose a reason for hiding this comment

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

Here and in other places that refer to Typescript:

Suggested change
activeTabErrors.Typescript = error;
activeTabErrors.TypeScript = error;

fixer: undefined,
location: '',
message,
severity: 8,
Copy link
Member

Choose a reason for hiding this comment

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

[Docs] It's not clear where this magic number comes from at first. Keeping in line with...

message.severity === 2
? 8 // MarkerSeverity.Error
: 4, // MarkerSeverity.Warning

Suggested change
severity: 8,
severity: 8, // MarkerSeverity.Error

Comment on lines +175 to +176
.split('\n')
.map((message: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

Do you have an example of a message that needs to be split? I've only been able to make versions with a single reported error at a time.

Comment on lines +178 to +182
fixer: undefined,
location: '',
message,
severity: 8,
suggestions: [],
Copy link
Member

Choose a reason for hiding this comment

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

[Refactor] Suggestion: what do you think about making these empty/falsy values optional in the ErrorItem type? IMO if they don't need to always have a real value, they should be allowed to not exist.

Suggested change
fixer: undefined,
location: '',
message,
severity: 8,
suggestions: [],
message,
severity: 8,

Comment on lines +92 to +93
Object.fromEntries(prev[activeTab].map(error => [error.group, error]))
.Typescript;
Copy link
Member

Choose a reason for hiding this comment

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

[Refactor] This confused me at first - we don't really need a whole grouped object, just the first tsconfig error.

Suggested change
Object.fromEntries(prev[activeTab].map(error => [error.group, error]))
.Typescript;
Object.values(prev[activeTab]).find(
error => error.group === 'TypeScript',
);

In fact, we don't care about this at all if errors.length, so this callback body can be rewritten like:

const tsconfigErrors =
  activeTab === 'tsconfig' &&
  !errors.length &&
  Object.values(prev[activeTab]).filter(
    error => error.group === 'TypeScript',
  );

return {
  ...prev,
  [activeTab]: tsconfigErrors || errors,
};

Copy link
Member

Choose a reason for hiding this comment

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

[Bug] These changes properly show a new error 👍. But when the user changes their tsconfig tab contents to no longer have an error, the shown error stays.

Repro steps:

  1. Create a playground with an error
  2. Modify the text in the tsconfig tab to no longer have the error: playground with modified state, no error
Screen.Recording.2025-03-31.at.8.44.37.AM.mov

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Issues waiting for a reply from the OP or another party
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Display tsconfig errors in playground
3 participants