Skip to content

feat: provide snippets for attribute #1509

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 14, 2021
Merged

feat: provide snippets for attribute #1509

merged 1 commit into from
Oct 14, 2021

Conversation

ivanwonder
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Oct 1, 2021
atscott
atscott reviewed Oct 1, 2021
View reviewed changes
Copy link
Collaborator

@atscott atscott left a comment

Choose a reason for hiding this comment

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

BTW, I just tried this out and the completion experience is really nice. Thanks for all your hard work!

@ivanwonder ivanwonder force-pushed the provide-snippets-for-attribute branch from 96878de to a0be535 Compare October 6, 2021 10:21
ivanwonder
ivanwonder commented Oct 6, 2021
View reviewed changes
Copy link
Contributor Author

@ivanwonder ivanwonder left a comment

Choose a reason for hiding this comment

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

Only add two tests. I think no need to copy all the tests from @angular/language-service

tsconfig.json Outdated
@@ -4,7 +4,8 @@
"module": "commonjs",
"moduleResolution": "node",
"sourceMap": true,
"strict": true
"strict": true,
"esModuleInterop": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

consistent with the angular repository.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 Yea, I was having an issue with this yesterday as well. I'll create a separate PR to make this change.

atscott
atscott requested changes Oct 6, 2021
View reviewed changes
tsconfig.json Outdated
@@ -4,7 +4,8 @@
"module": "commonjs",
"moduleResolution": "node",
"sourceMap": true,
"strict": true
"strict": true,
"esModuleInterop": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 Yea, I was having an issue with this yesterday as well. I'll create a separate PR to make this change.

package.json Outdated
"angular.suggest.includeCompletionsWithSnippetText": {
"type": "boolean",
"default": true,
"description": "Enable/disable snippet completions from angular language Server. Requires using TypeScript 4.3+ in the workspace."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the part about requiring TS 4.3+ accurate? Wouldn't the feature simply require the client to have snippetSupport in the capabilities? Or have I forgotten something on the @angular/language-service side of things that requires TS 4.3?

This feature does require that the "legacy View Engine" option is not enabled though (same with includeAutomaticOptionalChainCompletions, but I missed that in the last review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The TSServer support the includeCompletionsWithSnippetText since 4.3+.
And the user can override the version of ts(Maybe we should remove it like angular.ngdk).
I think I can remove this part and control the version by MIN_TS_VERSION.

vscode-ng-language-service/server/src/version_provider.ts

Lines 14 to 15 in f18b5ac

const MIN_TS_VERSION = '4.2';
const MIN_NG_VERSION = '12.0';

(I think the MIN_NG_VERSION is useless after all the @angular/language-service is always bundled with the extension).

@atscott atscott added the target: minor This PR is targeted for the next minor release label Oct 12, 2021
@atscott
Copy link
Collaborator

atscott commented Oct 12, 2021

@ivanwonder The build/test issues have been resolved. If you rebase your PR, we can now get this reviewed and merged.

@ivanwonder ivanwonder force-pushed the provide-snippets-for-attribute branch from 7539c2e to 97da874 Compare October 13, 2021 01:05
@ivanwonder ivanwonder force-pushed the provide-snippets-for-attribute branch from 90cd843 to 345b46e Compare October 13, 2021 14:24
@ivanwonder ivanwonder marked this pull request as ready for review October 13, 2021 14:27
@ivanwonder ivanwonder requested a review from atscott October 13, 2021 14:28
atscott
atscott reviewed Oct 13, 2021
View reviewed changes
Copy link
Collaborator

@atscott atscott left a comment

Choose a reason for hiding this comment

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

LGTM other than the one question about

includeCompletionsWithInsertText: this.includeAutomaticOptionalChainCompletions || includeCompletionsWithSnippetText,

@ivanwonder
Copy link
Contributor Author

BTW, Should we specify the version of node in package.json? I use v12.22 and do not know why the test in ivy_spec.ts are all failed. So I update to v16.

@atscott
Copy link
Collaborator

atscott commented Oct 14, 2021

BTW, Should we specify the version of node in package.json? I use v12.22 and do not know why the test in ivy_spec.ts are all failed. So I update to v16.

Yes, we should probably match what is done in the angular/angular repo with engines in the package.json and a .nvmrc file with the same version. Let's do that in another PR.

@atscott atscott added the action: merge Ready to merge label Oct 14, 2021
@atscott atscott merged commit 0428c31 into angular:master Oct 14, 2021
@ivanwonder ivanwonder deleted the provide-snippets-for-attribute branch October 15, 2021 01:05
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott atscott approved these changes

Assignees
No one assigned
Labels
action: merge Ready to merge cla: yes target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivanwonder @atscott