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

feat(flow): flow-operations tries to spread inexact objects #8000 #15

Conversation

Athelian
Copy link
Contributor

@Athelian Athelian commented Nov 9, 2022

🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.

Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of the pull request.

Description

Flow-operations tries to spread exact/inexact objects into the same merged object which leads to flow errrors.
I have already been running with this change in a custom plugin for quite some time.

Related issue #8000

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Screenshots/Sandbox (if appropriate/relevant):

Flow Try demonstrating the issue

How Has This Been Tested?

Pre-existing test demonstrates difference between useFlowExactObjects=false/true

Test Environment:

  • OS: Linux
  • Flow: 0.181.2
  • @graphql-codegen/flow-operations version: ^2.2.13

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

Forked from legacy repo

@charlypoly
Copy link
Collaborator

Hi @Athelian,

Thank you for your contribution! ⚡

Could you check the following:

  • fix ESLint issues (see PR files)
  • Locally run yarn generate:examples and commit the diff (to fix the dev-tests)

Thank you!

@Athelian
Copy link
Contributor Author

Locally run yarn generate:examples and commit the diff (to fix the dev-tests)

Hi @charlypoly.

I think yarn generate:examples is failing on main, would you need extra hands on it?

And please lmk if you would prefer I rebase this PR instead of merge.

Thank you.

@saihaj
Copy link
Collaborator

saihaj commented Sep 25, 2023

Hey @Athelian can you please rebase this PR and hopefully that should make all tests pass then we can merge

@saihaj saihaj added the waiting-for-answer Waiting for answer from author label Sep 25, 2023
@Athelian Athelian force-pushed the config.useFlowExactObjects-also-makes-top-level-of-fragment-exact branch from e68477d to 6adb328 Compare December 9, 2023 09:48
Copy link

changeset-bot bot commented Dec 9, 2023

🦋 Changeset detected

Latest commit: f522b78

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

This PR includes changesets to release 1 package
Name Type
@graphql-codegen/flow-operations 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

@Athelian Athelian marked this pull request as draft December 9, 2023 09:49
@Athelian Athelian marked this pull request as ready for review December 9, 2023 09:50
@Athelian
Copy link
Contributor Author

Athelian commented Dec 9, 2023

Hey @Athelian can you please rebase this PR and hopefully that should make all tests pass then we can merge

Done!

@saihaj
Copy link
Collaborator

saihaj commented Feb 2, 2024

hey @Athelian can you please add a test for this change and also run yarn changeset and create a patch which helps with release. You can learn here about changeset

@Athelian
Copy link
Contributor Author

hey @Athelian can you please add a test for this change and also run yarn changeset and create a patch which helps with release. You can learn here about changeset

Hi @saihaj I added the changeset.

As for a new test, I am not sure if it is necessary: All pre-existing tests needed updating, and all differences between true/false can covered by Should respect flow option useFlowExactObjects=false

@saihaj
Copy link
Collaborator

saihaj commented Feb 12, 2024

CI is still failing @Athelian

@Athelian
Copy link
Contributor Author

Athelian commented Feb 13, 2024

CI is still failing @Athelian

Failing CI looks working to me now if replicate in local.

Sorry for trouble.

Copy link
Collaborator

@saihaj saihaj left a comment

Choose a reason for hiding this comment

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

thank you for fixing!

@saihaj saihaj merged commit d13f6cc into dotansimha:main Feb 13, 2024
14 checks passed
@saihaj
Copy link
Collaborator

saihaj commented Feb 13, 2024

@graphql-codegen/[email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-answer Waiting for answer from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants