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: use Svelte parser for Svelte files #3204

Closed
wants to merge 1 commit into from

Conversation

tadeokondrak
Copy link

Using the Vue parser here leads to broken behavior when files use more
than basic Svelte syntax.

Using the Vue parser here leads to broken behavior when files use more
than basic Svelte syntax.
@changeset-bot
Copy link

changeset-bot bot commented May 31, 2023

⚠️ No Changeset found

Latest commit: 7e49b0d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@linux-foundation-easycla
Copy link

CLA Not Signed

@@ -41,6 +41,9 @@
"@babel/types": "^7.21.2",
"@graphql-tools/load": "^7.5.3",
"@vue/compiler-sfc": "^3.2.41",
"typescript": "^4.8",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why put typescript in dependency and not in devDependencies?

Copy link
Author

Choose a reason for hiding this comment

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

svelte2tsx or one of its dependencies calls methods in typescript that were added in 4.8.

@dimaMachina
Copy link
Collaborator

could you sign CLA and add some tests to https://github.com/graphql/graphiql/blob/main/packages/graphql-language-service-server/src/__tests__/findGraphQLTags-test.ts so there will be no regression in the future?

@tadeokondrak
Copy link
Author

Can't get the CLA site to work unfortunately

@acao acao reopened this Jun 4, 2023
@acao
Copy link
Member

acao commented Jun 4, 2023

@tadeokondrak just make sure your git commits use the same email you signed the CLA with!

@tadeokondrak
Copy link
Author

That isn't the issue - downloading the PDF of the agreement gives me a .pdf file containing only the following:

{"Message":"applying watermark failed : pdfcpu: validateStringEntry: dict=FreeText entry=DA invalid type","x-request-id":"c618f34a-b516-4fe5-9e77-5bb5af850624"}

Sorry, but I'd rather not go through the CLA support process to make this contribution.

@tadeokondrak
Copy link
Author

No change (site is still not showing me the individual contributor agreement) - closing

@jorydotcom
Copy link

@tadeokondrak I have filed an issue with the EasyCLA team to see if we can get this resolved for you.

@jorydotcom
Copy link

@tadeokondrak the CLA team confirmed your error and has pushed a fix; would you be willing to give it another go? We appreciate your contribution!

@acao
Copy link
Member

acao commented Jul 24, 2023

@tadeokondrak reviving this PR as a new PR now, can you give me some test cases where svelte crashes?

@acao acao mentioned this pull request Jul 24, 2023
1 task
@tadeokondrak
Copy link
Author

reviving this PR as a new PR now

Great!

can you give me some test cases where svelte crashes?

Not sure why, but this breaks with the old parser:

<script>
	graphql(`
		mutation Test {
			echo(m: "message")
		}
	`)
</script>

<div>
	<div>Text</div>
</div>

@acao
Copy link
Member

acao commented Jul 24, 2023

ah thank you! i will add this to the test. my guess is that is because vue expects <template> tags around html, whereas svelte in this case would parse anything outside of <script/> as html

@acao
Copy link
Member

acao commented Jan 13, 2024

ah, this introduced a bug with typescript module

@acao
Copy link
Member

acao commented Jan 13, 2024

@tadeokondrak unfortunately using svelte2tsx introduces bundling issues with the typescript peer dependency. this isn't revealed with our unit tests, or by the local dev runner or even the vscode test runner, because it requires manually testing with the bundled vsix extension

I am going to have to revert this change so that we can get the LSP server working again. You are welcome to re-create this PR again either without using a dependency with a peer dependency on typescript, or using svelte2tsx in a way that you can prove works with the bundled vscode-graphql vsix

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.

4 participants