-
Notifications
You must be signed in to change notification settings - Fork 352
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: support comments in vscode settings #5436
Conversation
📊 Benchmark resultsComparing with b44eb25 Package size: 260 MB⬆️ 0.08% increase vs. b44eb25
Legend
|
src/recipes/vscode/settings.mjs
Outdated
@@ -1,17 +1,17 @@ | |||
import { mkdir, readFile, stat, writeFile } from 'fs/promises' | |||
import { dirname, relative } from 'path' | |||
|
|||
import CommentJSON from 'comment-json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works. I've updated it.
Thank you so much for taking that on! I'd love to see some tests to prevent any future regressions 🙏🏻 |
Will add a test for JSON with comments. |
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']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spreading the array here since that converts it from the CommentArray type to a native JS array for comparison.
{ | ||
question: 'The Deno VS Code extension is recommended. Would you like to install it now?', | ||
answer: answerWithValue(NO), | ||
}, | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
@kaelig added the test. |
src/recipes/vscode/settings.mjs
Outdated
@@ -1,17 +1,17 @@ | |||
import { mkdir, readFile, stat, writeFile } from 'fs/promises' | |||
import { dirname, relative } from 'path' | |||
|
|||
import { parse, assign, stringify } from 'comment-json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do import * as JSSONC from "json-comments"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess that should work.
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
src/recipes/vscode/settings.mjs
Outdated
@@ -42,7 +42,7 @@ export const getSettings = async (settingsPath) => { | |||
|
|||
return { | |||
fileExists: true, | |||
settings: JSON.parse(file), | |||
settings: parse(file.toString()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than doing a toString()
here, I think you can do await readFile(settingsPath, "utf8")
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That seems better.
@danez I think we can merge this now. Also, please see: https://github.com/netlify/cli/pull/5436/files#r1096563844 |
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #5407
This PR adds support for having comments in the
settings.json
file used by VSCode while using thenetlify recipes vscode
command.For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)