Skip to content
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

refactor(language-core): make embeddedCodes optional in VirtualCode #137

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

remcohaszing
Copy link
Member

In many cases embeddedCodes is always empty. It might as well be optional.

In many cases `embeddedCodes` is always empty. It might as well be
optional.
@rchl
Copy link
Contributor

rchl commented Feb 20, 2024

Breaking change (maybe someone relies on it being an empty array and uses length check)? If so, is it really worth changing?

@remcohaszing
Copy link
Member Author

For Volar VirtualCode is input. volar-services doesn’t use embeddedCodes anywhere. So this could indeed be a breaking change, but only on a type level. This is easy to resolve using a non-null assertion or optional chaining.

@johnsoncodehk johnsoncodehk changed the title Make embeddedCodes optional in VirtualCode refactor(language-core): make embeddedCodes optional in VirtualCode Feb 20, 2024
@johnsoncodehk
Copy link
Member

johnsoncodehk commented Feb 20, 2024

I agree that embeddedCodes should be optional, the purpose of it being required in the past was to guide developers to implement embedded code (due to lack of documentation). But I'm sure we'll make progress on the documentation soon.

Since this is a breaking change, this will be released in 2.1 (Volar does not follow semantic versioning)

@johnsoncodehk johnsoncodehk merged commit d18261b into master Feb 20, 2024
6 checks passed
@remcohaszing remcohaszing deleted the embedded-codes-optional branch February 20, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants