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

chore(website): [playground] add copy as json and simplify ast viewer #6728

Merged
merged 26 commits into from Apr 2, 2023

Conversation

armano2
Copy link
Member

@armano2 armano2 commented Mar 21, 2023

PR Checklist

Overview

migrate ast viewer changes from #6656

this change is required to implement type viewer

notable changes:

  • improved accessibility
  • code is simplified and prepared to display other json like structures
  • show additional flags (languageVersion, languageVariant, scriptKind, transformFlags)
    image
  • iterable elements are now properly displayed
    image
  • copy as json is now available
    image

- add copy as json
- ast viewer is now more generic
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @armano2!

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.

@netlify
Copy link

netlify bot commented Mar 21, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit c0e6213
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/6429c917622ead0009a702c3
😎 Deploy Preview https://deploy-preview-6728--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@nx-cloud
Copy link

nx-cloud bot commented Mar 21, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c0e6213. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 30 targets

Sent with 💌 from NxCloud.

@armano2 armano2 linked an issue Mar 21, 2023 that may be closed by this pull request
2 tasks
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.

I started reviewing but then couldn't get the demo to work locally - is this just me? 😞

Screenshot of the playground's AST pane showing only 'undefined'

packages/website/src/hooks/useClipboard.ts Outdated Show resolved Hide resolved
packages/website/src/components/inputs/Tooltip.tsx Outdated Show resolved Hide resolved
packages/website/src/hooks/useDebouncedToggle.ts Outdated Show resolved Hide resolved
packages/website/src/components/ast/Elements.tsx Outdated Show resolved Hide resolved
packages/website/src/hooks/useClipboard.ts Outdated Show resolved Hide resolved
@armano2
Copy link
Member Author

armano2 commented Mar 22, 2023

I started reviewing but then couldn't get the demo to work locally - is this just me? 😞

not sure about what issue you have during local dev, but there was actual issue with undefined, i missed one thing while moving this change to standalone PR

@armano2 armano2 changed the title chore(website): [ast-viewer] add copy as json and simplify code chore(website): [playground] add copy as json and simplify ast viewer Mar 26, 2023
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #6728 (c0e6213) into v6 (6a307f0) will increase coverage by 2.31%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##               v6    #6728      +/-   ##
==========================================
+ Coverage   85.15%   87.46%   +2.31%     
==========================================
  Files         383      374       -9     
  Lines       13026    12879     -147     
  Branches     3839     3811      -28     
==========================================
+ Hits        11092    11265     +173     
+ Misses       1572     1229     -343     
- Partials      362      385      +23     
Flag Coverage Δ
unittest 87.46% <ø> (+2.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 166 files with indirect coverage changes

@armano2
Copy link
Member Author

armano2 commented Mar 29, 2023

i did some profiling on update tree, and I refactored code to be a little faster
issue with re-renders when you change tabs has been fixed

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.

Looks wonderful, thanks! I'm real stoked about this one in particular for the AST copying 😄

Requesting changes on a few small touchups - but nothing I'm particularly passionate about.

packages/website/src/components/Playground.tsx Outdated Show resolved Hide resolved
ScriptKind: extractEnum(window.ts.ScriptKind),
ScriptTarget: extractEnum(window.ts.ScriptTarget),
LanguageVariant: extractEnum(window.ts.LanguageVariant),
// @ts-expect-error: non public API
Copy link
Member

Choose a reason for hiding this comment

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

[Docs] Is there a tracking issue on TypeScript to expose this? (I couldn't find one) If not, could you please file one? And either way, link the issue here?

Copy link
Member Author

@armano2 armano2 Apr 2, 2023

Choose a reason for hiding this comment

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

this is intentionally a private API, and normally ts doesn't expose properties that have this in their types.

ast viewer actually prints both public and internal properties of object's and some of them normally are accessible trough functions

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm thinking if it's a API that we care about & would want to show, it'd make sense to ask for it to be turned public.

packages/website/src/components/ast/tsUtils.ts Outdated Show resolved Hide resolved
packages/website/src/components/ast/tsUtils.ts Outdated Show resolved Hide resolved
packages/website/src/components/ast/tsUtils.ts Outdated Show resolved Hide resolved
packages/website/src/components/ast/tsUtils.ts Outdated Show resolved Hide resolved
packages/website/src/components/ast/selectedRange.ts Outdated Show resolved Hide resolved
packages/website/src/components/ast/selectedRange.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 2, 2023
@armano2 armano2 removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 2, 2023
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.

Yay!

@armano2 armano2 merged commit 0d8d9f2 into v6 Apr 2, 2023
42 checks passed
@armano2 armano2 deleted the chore/website-improve-ast-viewer branch April 2, 2023 18:59
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Website: [Playground] Add "copy as JSON" option to AST Viewer
2 participants