feat: support comments in vscode settings #5436

tinfoil-knight · 2023-01-28T13:18:39Z

🎉 Thanks for submitting a pull request! 🎉

Summary

This PR adds support for having comments in the settings.json file used by VSCode while using the netlify recipes vscode command.

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.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

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

github-actions · 2023-01-28T13:20:08Z

📊 Benchmark results

Comparing with b44eb25

Package size: 260 MB

⬆️ 0.08% increase vs. b44eb25

^  265 MB  265 MB  265 MB  266 MB  265 MB  266 MB  266 MB  261 MB  262 MB  262 MB  260 MB  260 MB  260 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-30 (8146b3d): 272 MB
T-29 (43e6f9e): 272 MB
T-28 (4600e9b): 265 MB
T-27 (b0a6c85): 265 MB
T-26 (70ef88c): 265 MB
T-25 (cfcee54): 265 MB
T-24 (63d415b): 265 MB
T-23 (83a81b1): 265 MB
T-22 (8fe8339): 265 MB
T-21 (21f413c): 265 MB
T-20 (68caa51): 265 MB
T-19 (78a1932): 265 MB
T-18 (5eff7c1): 265 MB
T-17 (3768f87): 265 MB
T-16 (159c7c8): 265 MB
T-15 (d936220): 265 MB
T-14 (cd9ebf8): 265 MB
T-13 (6cee5bc): 265 MB
T-12 (cec55d1): 265 MB
T-11 (0929141): 265 MB
T-10 (d595d5f): 265 MB
T-9 (6297f84): 266 MB
T-8 (d9b54d9): 265 MB
T-7 (fc01b72): 266 MB
T-6 (81e7cc1): 266 MB
T-5 (055055a): 261 MB
T-4 (ec16aac): 262 MB
T-3 (8d01056): 262 MB
T-2 (0c781d6): 260 MB
T-1 (b44eb25): 260 MB
T (current commit): 260 MB

danez · 2023-01-30T10:52:56Z

src/recipes/vscode/settings.mjs

@@ -1,17 +1,17 @@
 import { mkdir, readFile, stat, writeFile } from 'fs/promises'
 import { dirname, relative } from 'path'

+import CommentJSON from 'comment-json'


Looking at the types of comment-json, it looks like it does not have a default export: https://github.com/kaelzhang/node-comment-json/blob/master/index.d.ts#L106

could we instead do:

import { assign, parse, stringify } from 'comment-json'

Or does that not work? The lib is written in commonjs, but at a quick glance it should work.

It works. I've updated it.

kaelig · 2023-01-30T18:05:38Z

Thank you so much for taking that on! I'd love to see some tests to prevent any future regressions 🙏🏻

tinfoil-knight · 2023-01-31T17:14:13Z

I'd love to see some tests to prevent any future regressions 🙏🏻

Will add a test for JSON with comments.

tinfoil-knight · 2023-02-04T17:24:41Z

tests/integration/640.command.recipes.test.cjs

+    t.is(settings.someSetting, 'value')
+    t.is(settings['deno.enable'], true)
+    t.is(settings['deno.importMap'], '.netlify/edge-functions-import-map.json')
+    t.deepEqual([...settings['deno.enablePaths']], ['/some/path', 'netlify/edge-functions'])


Spreading the array here since that converts it from the CommentArray type to a native JS array for comparison.

tinfoil-knight · 2023-02-04T17:27:24Z

tests/integration/640.command.recipes.test.cjs

+      {
+        question: 'The Deno VS Code extension is recommended. Would you like to install it now?',
+        answer: answerWithValue(NO),
+      },
+    ])


@danez Note here:

The Deno VS Code extension is recommended. Would you like to install it now? prompt is there on my local system but it seems like this question is not asked during the CI run. The test just ends up stuck on local env & didn't run before adding the question.

I've added the prompt for this test but this issue is there for other tests too. Should I add the extra prompt for other tests too (wherever applicable) in a separate PR?

tinfoil-knight · 2023-02-04T17:54:49Z

@kaelig added the test.

tests/integration/640.command.recipes.test.cjs

eduardoboucas · 2023-02-06T10:41:55Z

src/recipes/vscode/settings.mjs

@@ -1,17 +1,17 @@
 import { mkdir, readFile, stat, writeFile } from 'fs/promises'
 import { dirname, relative } from 'path'

+import { parse, assign, stringify } from 'comment-json'


This is totally down to personal preference, but I would suggest having a single JSONC import here and then use it throughout the file as JSONC.parse, JSONC.stringify, etc.

Seeing a function called stringify or parse doesn't give a lot of context and makes the code a little bit harder to parse, IMO.

It was a single CommentJSON import earlier which is what I prefer. @danez suggested otherwise.

The separate functions seem slightly confusing to me too without any context. WDYT @danez?

The problem is that the TS types say that these are named exports. https://github.com/kaelzhang/node-comment-json/blob/master/index.d.ts#L83 So as soon as we switch to TS this will not work, because TS would say no default export.

Could we do import * as JSSONC from "json-comments"?

Yeah I guess that should work.

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

eduardoboucas · 2023-02-06T10:59:56Z

src/recipes/vscode/settings.mjs

@@ -42,7 +42,7 @@ export const getSettings = async (settingsPath) => {

    return {
      fileExists: true,
-      settings: JSON.parse(file),
+      settings: parse(file.toString()),


Rather than doing a toString() here, I think you can do await readFile(settingsPath, "utf8") above?

Yeah. That seems better.

…contents

tinfoil-knight · 2023-02-09T17:17:48Z

@danez I think we can merge this now. Also, please see: https://github.com/netlify/cli/pull/5436/files#r1096563844

feat: support comments in vscode settings

4f2540b

tinfoil-knight self-assigned this Jan 28, 2023

tinfoil-knight added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jan 28, 2023

tinfoil-knight requested a review from kaelig January 28, 2023 14:20

tinfoil-knight marked this pull request as ready for review January 28, 2023 14:21

tinfoil-knight requested review from eduardoboucas and danez January 28, 2023 14:22

danez suggested changes Jan 30, 2023

View reviewed changes

refactor: destructure imports

2f778c1

tinfoil-knight added 2 commits February 4, 2023 21:47

Merge branch 'main' into 5407-comment-json

02de477

test: add test to check if JSON with comments is handled

452dea2

tinfoil-knight commented Feb 4, 2023

View reviewed changes

fix: remove unnecessary log statement

1dbee1a

tinfoil-knight requested a review from danez February 4, 2023 17:48

eduardoboucas reviewed Feb 6, 2023

View reviewed changes

test: remove .only modifier

b0c3a08

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

eduardoboucas reviewed Feb 6, 2023

View reviewed changes

tinfoil-knight added 2 commits February 6, 2023 16:38

refactor: use utf8 encoding in readFile instead of using toString on …

26ec300

…contents

refactor: import comment-json using a namespace

04bef8d

danez added the automerge Add to Kodiak auto merge queue label Feb 9, 2023

danez approved these changes Feb 9, 2023

View reviewed changes

Merge branch 'main' into 5407-comment-json

7c98336

kodiakhq bot merged commit 233bb57 into netlify:main Feb 9, 2023

token-generator-app bot mentioned this pull request Feb 9, 2023

chore(main): release 12.12.0 #5458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support comments in vscode settings #5436

feat: support comments in vscode settings #5436

tinfoil-knight commented Jan 28, 2023 •

edited

github-actions bot commented Jan 28, 2023 •

edited

danez Jan 30, 2023 •

edited

tinfoil-knight Jan 30, 2023 •

edited

kaelig commented Jan 30, 2023

tinfoil-knight commented Jan 31, 2023

tinfoil-knight Feb 4, 2023

tinfoil-knight Feb 4, 2023 •

edited

tinfoil-knight commented Feb 4, 2023

eduardoboucas Feb 6, 2023

tinfoil-knight Feb 6, 2023

danez Feb 6, 2023

eduardoboucas Feb 6, 2023

danez Feb 9, 2023

eduardoboucas Feb 6, 2023

tinfoil-knight Feb 6, 2023

tinfoil-knight commented Feb 9, 2023 •

edited

feat: support comments in vscode settings #5436

feat: support comments in vscode settings #5436

Conversation

tinfoil-knight commented Jan 28, 2023 • edited

Summary

github-actions bot commented Jan 28, 2023 • edited

📊 Benchmark results

Package size: 260 MB

danez Jan 30, 2023 • edited

Choose a reason for hiding this comment

tinfoil-knight Jan 30, 2023 • edited

Choose a reason for hiding this comment

kaelig commented Jan 30, 2023

tinfoil-knight commented Jan 31, 2023

Choose a reason for hiding this comment

tinfoil-knight Feb 4, 2023 • edited

Choose a reason for hiding this comment

tinfoil-knight commented Feb 4, 2023

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

tinfoil-knight commented Feb 9, 2023 • edited

tinfoil-knight commented Jan 28, 2023 •

edited

github-actions bot commented Jan 28, 2023 •

edited

danez Jan 30, 2023 •

edited

tinfoil-knight Jan 30, 2023 •

edited

tinfoil-knight Feb 4, 2023 •

edited

tinfoil-knight commented Feb 9, 2023 •

edited