implement addNodeAddonIncludePaths #6731
Conversation
|
|
|
FYI, we might not get around to reviewing this till Wednesday. |
That's fine. We generally update the changelog ourselves before a release, e.g. https://github.com/microsoft/vscode-cpptools/pull/6674/files -- the reason is that modifying the changelog with every PR can cause a merge conflict with every commit, which is annoying.
Targeting the insiders branch seems fine and we can merge to master (insiders is almost identical to master currently).
No, I think it should be "resource". "machine" and "machine-overridable" scopes are for scopes that are not expected to be valid across different machines/OS's, such as file paths.
Seems okay to me. It could also be made local to the function or file scope, but I'm not familiar with the recommended TypeScript practice either. More comments on the way... |
There was a problem hiding this comment.
You should install the ESLint extension, and run the the "lint" npm script or "gulp lint". The linter enforces are our whitespace/style rules.
configurations.ts
24:7 error expected variable-declaration: 'pfs' to have a typedef (tslint:typedef) @typescript-eslint/tslint/config
399:1 error Expected indentation of 8 spaces but found 6 @typescript-eslint/indent
400:1 error Expected indentation of 8 spaces but found 6 @typescript-eslint/indent
400:86 error Unexpected trailing comma comma-dangle
403:66 error Unexpected space before the ':' @typescript-eslint/type-annotation-spacing
408:23 error expected variable-declaration: 'package_json' to have a typedef (tslint:typedef) @typescript-eslint/tslint/config
411:1 error Expected indentation of 20 spaces but found 18 @typescript-eslint/indent
412:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
413:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
413:27 error expected variable-declaration: 'stdout' to have a typedef (tslint:typedef) @typescript-eslint/tslint/config
414:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
414:23 error Expected { after 'if' condition curly
416:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
417:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
418:1 error Expected indentation of 28 spaces but found 26 @typescript-eslint/indent
419:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
420:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
421:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
422:1 error Expected indentation of 28 spaces but found 26 @typescript-eslint/indent
423:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
424:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
425:1 error Expected indentation of 28 spaces but found 26 @typescript-eslint/indent
426:1 error Expected indentation of 24 spaces but found 22 @typescript-eslint/indent
431:1 error Expected indentation of 12 spaces but found 10 @typescript-eslint/indent
✖ 24 problems (24 errors, 0 warnings)
21 errors and 0 warnings potentially fixable with the --fix option.
Process-wise, I think it's better if PR's always target |
The potential problem is that main might have breaking changes that don't work without unreleased binaries, while the insider branch is guaranteed to work with the 1.2.0-preview binaries. There shouldn't be any issue with merging from insiders to main, right? |
|
@sean-mcmanus @bobbrow - thank you for the feedback. i will make these changes this weekend (unless you have a need for them before that or i find a little spare time) as this is a side project of mine. i'm not sure how the lint errors came about; i have eslint and the extension installed. i didn't run the lint script because i presumed that the extension would use the same rules as the script. i'll run the script and take care of the errors too. |
That's fine. Our goal is to release 1.2.0-insiders some time next week, but I'm not sure what day yet, but this could go into the next release if it doesn't make that. Yeah, ESLint is supposed to squiggle the lines with issues without having to run the specific command. |
|
addressed with the exception of dynamic enabling of the setting. i'm having to dig around a bit to understand that. questions:
btw, my problem not catching the eslint problems was that i didn't notice that eslint wanted to ask me permission to use the locally installed version. |
|
thanks - this weekend.
…On Fri, Jan 8, 2021 at 10:36 AM Sean McManus ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Extension/src/LanguageServer/configurations.ts
<#6731 (comment)>
:
> @@ -317,6 +323,9 @@ export class CppProperties {
} else {
configuration.includePath = [defaultFolder];
}
+ // check settings.addNodeAddonIncludePaths?
IsUnset is used because in TypeScript values like "" also implicitly
convert to false in "if" statements. So using isUnset is not what you
want, since it just checks if the setting doesn't exist.
Looking more closely at the code, applyDefaultConfigurationValues maybe
be the wrong place for setting this. The reason the vcpkgIncludes is set
here is because we wanted it to only appear when users create a new default
c_cpp_properties.json, but in your case it sounds like you want it to
always be auto-added?
The correct spot may be updateServerOnFolderSettingsChange (I'm not 100%
sure).
Yeah, we currently call util.promtForReloadWindowDueToSettingsChange after
we get a response from cpptools from the setting change. I was referring to DefaultClient.onDidChangeSettings(event:
vscode.ConfigurationChangeEvent, isFirstClient: boolean): { [key: string]:
string }, which has some existing code that checks if certain settings
were changed.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6731 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA72PO7JLHRV34FXOA4GNJLSY5GDDANCNFSM4VQSNYZQ>
.
|
Yeah, dynamic enabling seemed tricky because it would involve having to somehow delay the processing until after addNodeAddonIncludeLocations has generated the nodeAddonIncludes , which I don't think we have any similar case/example that does that, so that's why I was thinking just letting the user know a reload was required was sufficient, particular since it's not expected for that setting to change frequently. Enabling should be simple if nodeAddonIncludes has the data already though (assuming you don't want it to try to regenerate that dynamically). "any" are used in cases where we don't have type info or if the object is a dynamic type. I think it would be better to just use fs.promises directly instead of defining 'pfs'. |
|
my bad. i'll switch it. it seemed more general.
…On Fri, Jan 8, 2021 at 11:01 AM Sean McManus ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Extension/src/LanguageServer/configurations.ts
<#6731 (comment)>
:
> + for (const dep in this.nodeAddonMap) {
+ if (dep in package_json.dependencies) {
+ const execCmd: string = this.nodeAddonMap[dep];
+ let stdout = await util.execChildProcess(execCmd, rootPath);
+ if (!stdout) continue dependencyLoop;
+
+ // cleanup newlines
+ if (stdout[stdout.length - 1] === "\n") {
+ stdout = stdout.slice(0, -1);
+ }
+ // node-addon-api returns a quoted string, e.g., '"/home/user/dir/node_modules/node-addon-api"'.
+ if (stdout[0] === "\"" && stdout[stdout.length - 1] === "\"") {
+ stdout = stdout.slice(1, -1);
+ }
+ if (stdout) {
+ this.nodeAddonIncludes.push(stdout);
path.resolve doesn't work because it uses a working path of VS Code, such
as C:\\Users\\<user>\\AppData\\Local\\Programs\\Microsoft VS Code\, and
not the user's workspace. Was there a reason path.join("${workspaceFolder}",
stdout) or something similar wouldn't work?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6731 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA72POZASNHREUZIVJJNCC3SY5JANANCNFSM4VQSNYZQ>
.
|
|
i appreciate your help; i will have more time to devote on the weekend; i'm sorry i haven't really given this the attention it deserves this week. |
Yeah, that's fine...we usually don't work on weekends so you'll have to wait till Monday for a response. |
- set at init and on changes - always read nan/node-addon-api paths
|
ok, just pushed changes. biggest changes:
(mostly allows changing on the fly) if the user adds and i need to clean up the feedback is appreciated. |
- make nodeAddonMap a local - fail silently - add nodeAddonIncludesFound()
|
ok, i think it's pretty close at this point.
|
|
The "unbound breakpoint" issue you were hitting may have been caused by the extension not being activated...opening a .cpp should trigger the activation. |
- remove experimental artifact
|
i believe that i have made all the changes you requested. let me know if you have any additional. |
| @@ -627,6 +696,10 @@ export class CppProperties { | |||
| const configuration: Configuration = this.configurationJson.configurations[i]; | |||
|
|
|||
| configuration.includePath = this.updateConfigurationStringArray(configuration.includePath, settings.defaultIncludePath, env); | |||
| if (settings.addNodeAddonIncludePaths) { | |||
There was a problem hiding this comment.
configuration.includePath is checked/modified in the code below under certain settings. The "unmodified" configuration.includePath should be saved to some variable here to be used later.
There was a problem hiding this comment.
if (!configuration.includePath && !!this.defaultIncludes) {
configuration.includePath = this.defaultIncludes;
}
should check the unmodified/original variable and then do something like
const includePath: string[] = configuration.includePath || [];
configuration.includePath = includePath.concat(this.defaultIncludes);
There was a problem hiding this comment.
thanks for this; i feel like a dog watching tv when i work in this codebase. everything is new and my focus is on a very tiny slice of what is going on.
| @@ -288,7 +291,8 @@ export class CppProperties { | |||
| } | |||
|
|
|||
| private applyDefaultIncludePathsAndFrameworks(): void { | |||
| if (this.configurationIncomplete && this.defaultIncludes && this.defaultFrameworks && this.vcpkgPathReady) { | |||
| if (this.configurationIncomplete && this.defaultIncludes && this.defaultFrameworks && this.vcpkgPathReady | |||
| && this.nodeAddonIncludePathsReady) { | |||
There was a problem hiding this comment.
This variable is no longer needed. It was only needed when the applyDefaultConfigurationValues was being used.
There was a problem hiding this comment.
yes, obvious when i see it. you have a much better handle on all the moving pieces than i do - thanks for spending time on this.
| error = e; | ||
| } | ||
| } | ||
| if (error && pdjFound) { |
There was a problem hiding this comment.
I think this should be something like
if (error) {
if (pdjFound) {
...
}
} else {
this.handleConfigurationChange();
}
i.e. to skip this.handleConfigurationChange when it's not needed.
There was a problem hiding this comment.
Or checking this.nodeAddonIncludes.length > 0 (i.e. in case one of the cases errors?).
There was a problem hiding this comment.
i think it is (conceivably) possible that both node-addon-api and nan are referenced in package.json, both requires succeed, but one of two util.checkDirectoryExists() fail.
but i am changing it as i think it's 1) not possible now given what node-addon-api and nan return and 2) a really tiny edge case going forward because it means the package has to return an invalid directory.
thank you.
| .then(pdj => {pdjFound = true; return JSON.parse(pdj); }) | ||
| .catch(e => (error = e)); | ||
|
|
||
| const pathToNode: string = which.sync("node"); |
There was a problem hiding this comment.
This should be inside the try { below (avoids calling which.sync when it's not needed).
| // if addNodeAddonIncludePaths was turned on but no includes have been found yet then 1) presume that nan | ||
| // or node-addon-api was installed so prompt for reload. | ||
| if (changedSettings["addNodeAddonIncludePaths"] && settings.addNodeAddonIncludePaths && this.configuration.nodeAddonIncludesFound() === 0) { | ||
| util.promptForReloadWindowDueToSettingsChange(); |
There was a problem hiding this comment.
This will only get shown if a user adds the nan/node-addon-api to their package.json and then enables the setting -- is that intentional? It seems fine to me, but it just covers a subset of the cases.
There was a problem hiding this comment.
prompts for reload if addNodeAddonIncludePaths is turned on but no addon includes were found by readNodeAddonIncludeLocations. this doesn't catch the case where nan was found but they added node-addon-api before turning on the setting.
above is my original comment on this. it's definitely a subset but seemed like one that is likely to be hit with a typical workflow - create a project, start editing, add node-addon-api or nan, notice the squiggles, and then fix it.
if your experience is different i'm happy to take it out. but it's kind of the best that could be done without creating a file-watcher or changing readNodeAddonIncludeLocations to be synchronous.
There was a problem hiding this comment.
Yeah, seems fine to me.
Yeah, I think it's close. I noticed some additional minor issues. |
- remove unused flag - avoid unnecessary work - handle alternate include setting path
|
@sean-mcmanus - thanks for the review. i think i've addressed the issues; i don't think the first one was minor and i appreciate your continuing help. |
Yeah, thanks for your contribution. Looks like it'll make 1.2.0-insiders -- hopefully it'll be available by end of day Thursday unless we find any last minute blocking issues. |
Merge to main: implement addNodeAddonIncludePaths (#6731)
This PR addresses the common cases for #4854 by adding the include paths specified by
nanandnode-addon-apiwhen they are listed as dependencies in package.json. This was the first time I've looked at vscode-cpptools code and it's also the first time I've written any typescript, so suggestions about how I can improve in either area are welcome.I haven't changed the changelog nor even looked at adding a specific test pending feedback. Afaict, it passes the unit tests and integration tests but I don't see any positive feedback to be sure.
I modified the insiders branch, so that is the target for this PR. Not sure whether that's the regular practice or not.
Questions:
settings.addNodeAddonIncludePathsor just concat an empty array configuration.ts 327pfs(fs/promises) is defined at the top of the file but only used one place configuration.ts 408. prefer it locally scoped?Obviously, you're welcome to change names, wording, etc. or ask me to do so.