feat: check for deno vs code ext and download when prompted #4698

JWhist · 2022-06-13T18:24:44Z

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes # https://github.com/netlify/pillar-runtime/issues/356

Adds a check on first run to see if user has Deno VS Code extension installed, and if not, prompts them to ask if they would like to install it. If they select yes, it installs the Deno VS Code Extension. This helps with some syntax highlighting.

Looking for a bit of help in testing this. I uninstalled my local version a few times and tried that way, but looking for advice on what would be a standard way to do this? Maybe a container or something? Do we have a docker image we use for such cases?

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

github-actions · 2022-06-13T18:25:40Z

📊 Benchmark results

Comparing with a67ec05

Package size: 226 MB

(no change)

^                                                          291 MB  294 MB  294 MB  294 MB  294 MB         
│                                                           ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐          
│                                                           |  |    |  |    |  |    |  |    |  |          
│                                                           |  |    |  |    |  |    |  |    |  |          
│ ──────────────────────────────────────────────────────────┼──┼────┼──┼────┼──┼────┼──┼────┼──┼──────────
│  226 MB  226 MB  226 MB  226 MB  226 MB  221 MB  221 MB   |  |    |  |    |  |    |  |    |  |   226 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    |  |    |  |    |  |    |  |    |  |    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T

Legend

T-18 (a67ec05): 226 MB
T-17 (7857dae): 226 MB
T-16 (b9e41ec): 226 MB
T-15 (2c58d10): 226 MB
T-14 (6523ede): 226 MB
T-13 (183429c): 226 MB
T-12 (b952831): 226 MB
T-11 (e9da0a7): 226 MB
T-10 (a8677ed): 226 MB
T-9 (3a79895): 226 MB
T-8 (d917fe2): 226 MB
T-7 (fd355b1): 221 MB
T-6 (b70313a): 221 MB
T-5 (8f64f93): 291 MB
T-4 (76f209f): 294 MB
T-3 (af6bc38): 294 MB
T-2 (8ded8fb): 294 MB
T-1 (33c931b): 294 MB
T (current commit): 226 MB

eduardoboucas

Looking good! I left some comments that I think we should address before merging.

Also, I think we should use the feat: prefix for this PR and not fix:, so that we generate a minor version and not a patch.

eduardoboucas · 2022-06-15T09:56:56Z

src/recipes/vscode/index.js

+}
+
+const getDenoExtPrompt = () => {
+  const message = 'The Deno VSCode extension is recommended.  Would you like to install it now?'


Suggested change

const message = 'The Deno VSCode extension is recommended. Would you like to install it now?'

const message = 'The Deno VSCode extension is recommended. Would you like to install it now?'

eduardoboucas · 2022-06-15T09:59:27Z

src/recipes/vscode/index.js

@@ -27,6 +29,30 @@ const getEdgeFunctionsPath = ({ config, repositoryRoot }) =>

 const getSettingsPath = (repositoryRoot) => join(repositoryRoot, '.vscode', 'settings.json')

+const hasDenoVSCodeExt = () => {
+  const ext = execSync('code --list-extensions', { encoding: 'utf-8' }).trim()


We should use execa to spawn subprocesses. You can search the codebase for it to see how it's being used.

Also, we should try to use asynchronous methods as much as possible (so we prefer execa over execaSync).

…ot installed

src/recipes/vscode/index.js

eduardoboucas

Looks great! Would you like to add some tests?

eduardoboucas · 2022-06-17T09:55:44Z

src/recipes/vscode/index.js

    log(
-      `${NETLIFYDEVWARN} If you don't have the Deno VS Code extension, install it by visiting ${chalk.blue(
+      `${NETLIFYDEVWARN} Unable to install Deno VS Code extension, install it by visiting ${chalk.blue(


Small change to make this a bit more friendly.

Suggested change

`${NETLIFYDEVWARN} Unable to install Deno VS Code extension, install it by visiting ${chalk.blue(

`${NETLIFYDEVWARN} Unable to install Deno VS Code extension. To install it manually, visit ${chalk.blue(

eduardoboucas · 2022-06-17T09:56:18Z

src/recipes/vscode/index.js

+}
+
+const getDenoVSCodeExt = async () => {
+  await execa('code', ['--install-extension', 'denoland.vscode-deno']).stdout.pipe(process.stdout)


Should we also pipe stderr?

I think we do not even need to manually pipe stdout (or stderr for this matter) as this is the default behavior of execa. At least by looking at the options: https://github.com/sindresorhus/execa#stdout-1

So if we remove the .stdout.pipe(...) part does it still work/log something?

In my local testing, if I remove the pipe commands it does not log anything, I would have to save the stdout from the response like const { stdout } = await execa... then console.log(stdout) etc. Which is a roundabout way of doing the same thing as piping it imo.

EDIT: just now found out about stdio: 'inherit' option and feel kind of silly

I think the default behaviour is to capture the output into the return value of the execa call. If you also want to pipe it into stdout/stderr, you need to make that explicit.

src/recipes/vscode/index.js

eduardoboucas · 2022-06-21T15:23:05Z

tests/integration/snapshots/600.framework-detection.test.js.md

@@ -242,4 +242,4 @@ Generated by [AVA](https://avajs.dev).
    > node -p process.env.GATSBY_LOGGER␊
    ␊
    yurnalist␊
-    ◈ "npm run develop" exited with code *. Shutting down Netlify Dev server`
+    .◈ "npm run develop" exited with code *. Shutting down Netlify Dev server`


What is this about?

A flaky test that I was trying to get around, there is a helper function that concats a series of loading dots .... down to a single dot. But occasionally the load happens fast enough to not have any dots at all (at least that is my theory). I was toying with the regex in the helper but gave up on it. This change was actually an error, since it's an autogenerated file anyway, and isn't going to help me pass the test at all.

Ah, I see! Do you want to revert before we merge?

Not necessary, I have another PR going to fix that test

Okay, but why would we merge this change? We should revert and let the other PR fix the test?

Ok, that makes sense. Sorry, I'm still in weekend mode!

eduardoboucas · 2022-06-21T15:23:43Z

src/recipes/vscode/index.js

@@ -27,6 +28,26 @@ const getEdgeFunctionsPath = ({ config, repositoryRoot }) =>

 const getSettingsPath = (repositoryRoot) => join(repositoryRoot, '.vscode', 'settings.json')

+const hasDenoVSCodeExt = async () => {
+  const { stdout: extensions } = await execa('code', ['--list-extensions'], { stderr: 'inherit' })


I'm curious why you've used inherit here and on L37?

I want to save the data from the extensions if the stdout is ok, but log the error to the screen if it is going to stderr.

Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>

eduardoboucas

Nice one! It'd be great to add some tests, even if that happens on a follow up PR. Have you given any thought to how you'd approach that?

JWhist · 2022-06-21T18:24:27Z

Nice one! It'd be great to add some tests, even if that happens on a follow up PR. Have you given any thought to how you'd approach that?

I wanted to add tests, but honestly I'm not quite sure how to approach it. Any advice?

fix: check for deno vs code ext and download when prompted

2c40db9

JWhist self-assigned this Jun 13, 2022

github-actions bot added the type: bug code to address defects in shipped code label Jun 13, 2022

JWhist added type: chore work needed to keep the product and development running smoothly and removed type: bug code to address defects in shipped code labels Jun 13, 2022

github-actions bot added the type: bug code to address defects in shipped code label Jun 14, 2022

JWhist marked this pull request as ready for review June 14, 2022 15:01

eduardoboucas requested changes Jun 15, 2022

View reviewed changes

JWhist added 2 commits June 15, 2022 10:13

fix: change execSync to use execa

7dcaf8d

fix: change execSync to use execa

bf428b7

JWhist changed the title ~~fix: check for deno vs code ext and download when prompted~~ feat: check for deno vs code ext and download when prompted Jun 15, 2022

github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jun 15, 2022

JWhist and others added 5 commits June 15, 2022 10:54

fix: catch error during extension install to fix tests or if vscode n…

8c7a65f

…ot installed

fix: add error catching

1e51be0

fix: bypass prompt when error checking for deno

a375a68

Merge branch 'main' into checkForDenoExt

59057d5

Merge branch 'main' into checkForDenoExt

f13c7be

JWhist requested a review from eduardoboucas June 15, 2022 16:30

eduardoboucas requested changes Jun 15, 2022

View reviewed changes

src/recipes/vscode/index.js Outdated Show resolved Hide resolved

fix: move error handling to single block

809e223

JWhist requested review from a team and eduardoboucas June 15, 2022 20:13

eduardoboucas reviewed Jun 17, 2022

View reviewed changes

JWhist and others added 4 commits June 17, 2022 10:46

fix: add stderr piping to execa command

8172bbe

fix: add stderr piping to execa commands

599053a

fix: dot diff in test

21777f6

Merge branch 'main' into checkForDenoExt

74e2a42

JWhist requested review from danez and eduardoboucas June 21, 2022 14:21

Merge branch 'main' into checkForDenoExt

bc21edf

eduardoboucas reviewed Jun 21, 2022

View reviewed changes

JWhist and others added 4 commits June 21, 2022 11:55

Update src/recipes/vscode/index.js

b761def

Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>

Merge branch 'main' into checkForDenoExt

bd4ca72

chore: revert snapshot change

a4acb0e

Merge branch 'main' into checkForDenoExt

6fadfee

eduardoboucas approved these changes Jun 21, 2022

View reviewed changes

JWhist merged commit b09b86e into main Jun 21, 2022

JWhist deleted the checkForDenoExt branch June 21, 2022 18:42

token-generator-app bot mentioned this pull request Jun 21, 2022

chore(main): release 10.6.0 #4695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: check for deno vs code ext and download when prompted #4698

feat: check for deno vs code ext and download when prompted #4698

JWhist commented Jun 13, 2022 •

edited

Loading

github-actions bot commented Jun 13, 2022 •

edited

Loading

eduardoboucas left a comment

eduardoboucas Jun 15, 2022

eduardoboucas Jun 15, 2022

eduardoboucas left a comment

eduardoboucas Jun 17, 2022

eduardoboucas Jun 17, 2022

danez Jun 17, 2022 •

edited

Loading

JWhist Jun 17, 2022 •

edited

Loading

eduardoboucas Jun 17, 2022

eduardoboucas Jun 21, 2022

JWhist Jun 21, 2022

eduardoboucas Jun 21, 2022

JWhist Jun 21, 2022

eduardoboucas Jun 21, 2022

JWhist Jun 21, 2022

eduardoboucas Jun 21, 2022

JWhist Jun 21, 2022

eduardoboucas left a comment

JWhist commented Jun 21, 2022

	const message = 'The Deno VSCode extension is recommended. Would you like to install it now?'
	const message = 'The Deno VSCode extension is recommended. Would you like to install it now?'

	`${NETLIFYDEVWARN} Unable to install Deno VS Code extension, install it by visiting ${chalk.blue(
	`${NETLIFYDEVWARN} Unable to install Deno VS Code extension. To install it manually, visit ${chalk.blue(

feat: check for deno vs code ext and download when prompted #4698

feat: check for deno vs code ext and download when prompted #4698

Conversation

JWhist commented Jun 13, 2022 • edited Loading

Summary

github-actions bot commented Jun 13, 2022 • edited Loading

📊 Benchmark results

Package size: 226 MB

eduardoboucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardoboucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danez Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

JWhist Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardoboucas left a comment

Choose a reason for hiding this comment

JWhist commented Jun 21, 2022

JWhist commented Jun 13, 2022 •

edited

Loading

github-actions bot commented Jun 13, 2022 •

edited

Loading

danez Jun 17, 2022 •

edited

Loading

JWhist Jun 17, 2022 •

edited

Loading