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: skip config updates when no custom filename is defined #1951

Merged
merged 2 commits into from
Oct 11, 2021

Conversation

GoodForOneFare
Copy link
Contributor

Skip config reload when no custom filename is defined. Previously, any project without a load.fileName configuration would reload configuration on every change event. This is because match(undefined) always returns true.

['graphql.config', 'graphqlrc', undefined].some(v => 'this-should-not-match'.match(v)?.length) // true

@changeset-bot
Copy link

changeset-bot bot commented Sep 17, 2021

🦋 Changeset detected

Latest commit: 0f8c350

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
graphql-language-service-server Patch

Not sure what this means? Click here to learn what changesets are.

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 17, 2021

CLA Signed

The committers are authorized under a signed CLA.

@acao
Copy link
Member

acao commented Sep 17, 2021

Thanks so much for opening a PR for this!

@GoodForOneFare GoodForOneFare changed the title Skip config updates when no custom filename is defined fix: skip config updates when no custom filename is defined Sep 20, 2021
@GoodForOneFare
Copy link
Contributor Author

@acao I think this is ready for review 🙏 Please let me know if I should be tagging anyone else in!

@p-szm
Copy link
Contributor

p-szm commented Oct 7, 2021

@acao @dotansimha any chance we can get this in? I'm running into the same bug

@acao
Copy link
Member

acao commented Oct 10, 2021

Really not sure what is wrong with github actions for this PR. When I get a chance, I will try lint/tests locally so we can get this one merged, high priority!

Another matter, but I think we also need to debounce config updates when editing a config file

@dotansimha
Copy link
Member

Really not sure what is wrong with github actions for this PR. When I get a chance, I will try lint/tests locally so we can get this one merged, high priority!

Another matter, but I think we also need to debounce config updates when editing a config file

I think it was an issue with GH Actions, not specific with this PR.
I pushed an empty commit and it seems to be running now :)

@codecov
Copy link

codecov bot commented Oct 10, 2021

Codecov Report

Merging #1951 (0f8c350) into main (2d91916) will increase coverage by 0.75%.
The diff coverage is 65.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1951      +/-   ##
==========================================
+ Coverage   65.70%   66.45%   +0.75%     
==========================================
  Files          85       86       +1     
  Lines        5106     5143      +37     
  Branches     1631     1640       +9     
==========================================
+ Hits         3355     3418      +63     
+ Misses       1747     1721      -26     
  Partials        4        4              
Impacted Files Coverage Δ
...ackages/graphiql-toolkit/src/create-fetcher/lib.ts 50.90% <20.00%> (-8.67%) ⬇️
packages/graphiql/src/utility/HistoryStore.ts 62.26% <62.26%> (ø)
packages/graphiql/src/components/QueryHistory.tsx 73.91% <76.47%> (+6.69%) ⬆️
...iql/src/components/DocExplorer/MarkdownContent.tsx 100.00% <100.00%> (ø)
packages/graphiql/src/components/GraphiQL.tsx 58.34% <100.00%> (+0.83%) ⬆️
...ql-language-service-server/src/MessageProcessor.ts 66.66% <100.00%> (+6.38%) ⬆️
...hql-language-service-server/src/findGraphQLTags.ts 67.64% <100.00%> (+7.00%) ⬆️
...raphql-language-service-server/src/GraphQLCache.ts 52.25% <0.00%> (+0.60%) ⬆️
packages/graphiql/src/utility/QueryStore.ts 42.85% <0.00%> (+2.04%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65c03d4...0f8c350. Read the comment docs.

@acao
Copy link
Member

acao commented Oct 11, 2021

thank you @dotansimha ! Yes I think this was just a day with outages. Merging this now :) great work @GoodForOneFare !!

@acao acao merged commit 72bff0e into graphql:main Oct 11, 2021
@github-actions github-actions bot mentioned this pull request Oct 11, 2021
@GoodForOneFare GoodForOneFare deleted the fix-config-loading branch October 11, 2021 16:11
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

4 participants