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

fix(language-core): try handle node next module resolution #3159

Merged
merged 11 commits into from Jul 26, 2023

Conversation

kalvenschraut
Copy link
Contributor

These are the changes I mentioned in #2662 in my last comment.

If direction is to not support node next then this PR can be closed and disregarded.

@kalvenschraut kalvenschraut changed the base branch from master to v1 May 11, 2023 14:07
@kalvenschraut kalvenschraut force-pushed the node-next-resolution branch 2 times, most recently from cfd430d to 9da85c6 Compare May 11, 2023 14:17
@kalvenschraut
Copy link
Contributor Author

this only seems to pass on the master branch, not sure what it is at this point and I don't think its needed to get this out right away since I personally decided to move to the bundler resolution anyway

@kalvenschraut kalvenschraut changed the base branch from v1 to master May 11, 2023 14:33
Copy link
Member

@yoyo930021 yoyo930021 left a comment

Choose a reason for hiding this comment

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

I will test in my project for this PR.

packages/vue-language-core/src/languageModule.ts Outdated Show resolved Hide resolved
packages/vue-language-core/src/languageModule.ts Outdated Show resolved Hide resolved
@johnsoncodehk
Copy link
Member

@yoyo930021 changes look neat, have you tested it in a real project?

@johnsoncodehk
Copy link
Member

You can revert master merge and I'll handle it, it can't just resolve in this PR.

@kalvenschraut
Copy link
Contributor Author

okay, undid my master merge and yea quickly realize there was some typing problems. Looks like the problem still exists on the action checks? Seems fine locally so guessing the volar package change and is probably what you were referring to what you needed to resolve?

Otherwise should be all cleaned up now with request changes.

@johnsoncodehk johnsoncodehk changed the base branch from master to v1 June 7, 2023 16:13
@johnsoncodehk johnsoncodehk changed the base branch from v1 to master June 7, 2023 16:14
@johnsoncodehk
Copy link
Member

This is difficult to solve in this repo, may need to move to https://github.com/volarjs/volar.js/blob/master/packages/typescript/src/languageServiceHost.ts, will continue to investigate.

johnsoncodehk added a commit to volarjs/volar.js that referenced this pull request Jul 26, 2023
@johnsoncodehk
Copy link
Member

Hey @kalvenschraut, I can't push commit to RTVision:node-next-resolution branch, can you check permissions?

@kalvenschraut
Copy link
Contributor Author

I know on newer pull requests I make there is a checkbox to allow the maintainers to do this, not sure how I can retroactively give permission. Best thing I could find is inviting you as a collaborator on the fork.

@johnsoncodehk
Copy link
Member

Thanks!

@johnsoncodehk johnsoncodehk merged commit 76241b0 into vuejs:master Jul 26, 2023
3 checks passed
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.

None yet

3 participants