-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Convert to pnpm to fix autocompletions #442
Conversation
Adds a patch so that we can don't get directive autocompletions on the line after a directive. Instead, we get the field autocompletion that the user was expecting.
To note: we get these warnings on install, but they can be fixed in a future pr:
|
Can you say more about the problems here and why we needed to switch? |
Reference: https://stackoverflow.com/a/73713976 So we now use only |
invariant(lines.length >= location.line, 'Query text must have more lines than where the error happened'); | ||
let stream = null; | ||
- for (let i = 0; i < location.line; i++) { | ||
+ for (let i = 0; i <= location.line; i++) { | ||
stream = new CharacterStream(lines[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem quite right to me. The invariant above only asserts that lines[i]
is valid if i < location.line
, not if i == location.line
. Either the invariant needs to change as well, or this is not the right change to make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I had to change the invariant in the pr: graphql/graphiql#3397
This isn't a huge problem for us, we can just get rid of this patch and wait until that pr is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(it does work in it's current form too though...)
By converting to pnpm we can now patch our dependencies which is amazing since most of our meaningful code lies in our dependencies.
Changes made in this pr:
Started depending on a bunch of new packages.
Explanation: We already depended on them, our package manager just wasn't aware of that since npm probably has some tricks pnpm doesn't, or something along those lines.
Update
react-router-dom
Explanation: It was causing some issues with pnpm, but I also don't think it causes any issues to update it in this pr.
Use
react-router-dom
inexperiments/browser_based_querying/src/QueryFragmentAdapter.tsx
Explanation: We used
react-router-dom
andreact-router
together, which caused problems for pnpm, so we had to pick one according to SO, and it was easier to move toreact-router-dom
.Patches I've added:
experiments/browser_based_querying/patches/[email protected]
=> The change toesm/interface/getAutocompleteSuggestions.js
will fix autocompletions giving a directive when you wanted to autocomplete a field. Should still allow autocompleting directives after fields.The change to
esm/interface/getDiagnostics.js
will close #285.experiments/browser_based_querying/patches/[email protected]
=> Stop randomly logging from the lexer.