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(www): playground #1182

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

feat(www): playground #1182

wants to merge 26 commits into from

Conversation

magurotuna
Copy link
Member

@magurotuna magurotuna commented Jul 30, 2023

This implements a playground to lint.deno.land where we can write random source code and see the diagnostics right away.

Currently the wasm file and its glue code are generated locally and committed to the repo, which means that we need to run the manual updating process (just running deno task wasmbuild) every time we make changes to the linter. We could automate it by moving this process inside CI, but this isn't done here because that would require changing build settings in Deno Deploy too. Instead, we add a step to CI to check if the wasm files committed to the repo are up-to-date.

You can try it out at https://deno-lint--playground.deno.dev/playground

Demo

CleanShot.2024-05-06.at.10.32.06.mp4

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

This will be nice! It would be good if we could not check in the Wasm build artifacts though.

www/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,240 @@
let wasm;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we could have this js file, the wasm file, and the .d.ts file built when creating the website? Then we don't need to keep updating these and they don't bloat the repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was thinking about that, and put it off for the moment to focus on the implementation part. I'll try to find a way to achieve continuous delivery.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about it a little bit further, I'd prefer to have developers commit the wasm files into the repo instead of relying on CI to build them, mostly because building them in CI would require configuration changes in Deno Deploy. Of course we could do that, but I think we can do so later if it's really needed (for instance we may want to do AoT build for fresh, which can be done together with wasm build in CI)

@sigmaSd
Copy link
Contributor

sigmaSd commented Sep 1, 2023

That's cool!

Couple of remarks:

  • Deno apis seems to be not understood by the lsp / same thing with top level awaits
  • Can we have an option to use all rules

@sigmaSd
Copy link
Contributor

sigmaSd commented Sep 1, 2023

@magurotuna
Copy link
Member Author

@sigmaSd Thanks for the feedback. What do you mean by "not understood by the lsp"? Like there's no completion options showing up when you put Deno. in the editor part of the playground?

@sigmaSd
Copy link
Contributor

sigmaSd commented Sep 1, 2023

It looks like this
image

@magurotuna
Copy link
Member Author

I made several updates and I think now it's ready for review again. PR description has been updated too, but here's a quick summary:

  • Add language select box specifically to support JSX(TSX) syntax
  • Add checkbox to enable all rules (default is recommended rules only)
  • Update color theme in monaco dynamically on users' preference (see the last part of the demo movie)

@@ -4,19 +4,25 @@
"build": "deno run -A dev.ts build",
"preview": "deno run -A main.ts"
},
"lock": false,
"lock": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for reviewers: using libraries from jsr seems to require this

Comment on lines -58 to -61
let maybe_file_path = ctx.specifier().to_file_path().ok();
let file_stem = maybe_file_path
.as_ref()
.and_then(|p| p.file_stem())
Copy link
Member Author

Choose a reason for hiding this comment

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

for reviewers: Url's to_file_path method isn't provided for wasm32-unknown-unknown. As a workaround, we extract the last segment from specifier's path part, and treat it as a file name.

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.

None yet

3 participants