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

[WIP] Added transfer-field transform #3041

Closed
wants to merge 4 commits into from

Conversation

ntziolis
Copy link
Contributor

@ntziolis ntziolis commented Oct 24, 2021

Description

This is an initial version of a transfer field transform.

Thigs left todo:

  • bare mode
    • disucss if we should batch (by type + action) calls to removeObjectFields + appendObjectFields
      • I already have an implementation for this in my local version
      • However this would introduce hidden reordering of compared to in which order the steps are defined in the config
      • And while I cant think of a used case where the order of executions should matter,
      • I would hate to limit the use of this transform given the limited benefit in performance
    • move as much init logic into ctor as possible
    • write more comprehensive unit tests
  • wrap mode
    • not implemented yet
    • that said @santino given how bare works currently removeObjectFields + appendObjectFields
      • wouldn't this also work for wrap mode?
      • meaning bare / wrap would have the same implementation in this rare instance?
      • or are those methods already using wrapping of some kind and they shouldn't have been used in bare more?
        • I used the hoist transform as inspiration but given these methods are not in the wrap, but utils package I assumed they are not using wrapping
  • documentation
    • add section on website for transform
  • add changeset

Open questions:

  • Should the transform be able to handle adding an output type if it doesn't exist yet
    • For example we have seen many REST APIs that only provide POST
    • This means that the source will not generate a Query type
    • When trying to move fields from Mutation to Query the transform would then fail

Addresses parts of #2975

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I have started adding unit tests, but def. will add more before the PR will be merged.

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 and linter rules pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

@santino Please let me know what you think about the general approach.

@changeset-bot
Copy link

changeset-bot bot commented Oct 24, 2021

⚠️ No Changeset found

Latest commit: 253fd33

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

@Lappihuan
Copy link

how am i able to test this?
I think my troubel is with the whole repository not with your transfrom.
after cloning this repo i tried setting up a example project inside the example directory but using pnpm install in the whole repo there is missing dependencies left and right.
I'm not able to build mesh so i cant reference the locally built packages in my package.json via workspace:...

@ntziolis
Copy link
Contributor Author

ntziolis commented Oct 31, 2021

The easiest way is to just create a unit test with you use case. You don't need to write any assert statements instead use the snapshot feature on the resulting Schema as this weil give you the whole signs in easily readable format.

In order to run the unit tests for a specific transform you might have to run npm Install in the transform root (I don't remember if that was required but I think so)

@Lappihuan
Copy link

i can't get it to install all dependencies, wether with npm, yarn nor pnpm.
there seem to be no instructions for this repo on how to set it up.

@ardatan
Copy link
Owner

ardatan commented Oct 31, 2021

This repo uses "yarn" workspaces so you should run "yarn install" in the root directory of this repo once. Then you should run "yarn build" to make examples work with the built code from the same repo.

We have this contributor guide referred in the README but we'd love to improve it together if it doesn't look helpful enough :/

@Lappihuan
Copy link

@ardatan thanks! that worked.
all i had to do was change "cp" in one of the package.json scripts to make it work on windows.

I think i was mixing stuff up with gqtys repo where i'm currently contributing something.

@ntziolis i was able to test your transfrom last night and i'm able to move fields from mutation to queris with the soap handler.
I didn't get to test the regex funcitonality but i will try to do that tonight.

To your open question, wouldn't it allready be possible to use the replace-fields transform on the transfered field?
Or is this a ordering issue?

@santino
Copy link
Contributor

santino commented Nov 3, 2021

Hi @ntziolis,
please do accept my apologies for not getting back to you earlier, it's been a pretty busy week.

I've looked at your implementation and you've done a good job.
I can see the setup works and is able to tackle most of the things we discussed.

I wanted to try playing with this to see how I would implement it, in order to push myself and see how things could be achieved differently.
I have been able to dedicate just a couple of hours today and I came up with a different way of setting up a transform that can transfer things across the schema, not just fields.
Taking inspiration from filter-schema transform I decided to use glob matching and realised this transform can be used to transfer (move/copy) arguments as well, in addition to fields.
I have then named it transfer-schema.

With little time spent on it, there is plenty more to do before this becomes ready, especially given the wide functionality that I have in mind.
But if you are interested, my initial commit can be seen here; disclaimer: it is still quite dirty, needs serious polishing.

You can especially see how I am adding some tests (they're not functioning yet, like in proper TDD) that explain what my plans are for this transform.

Will appreciate it if you can take a look and let me know your thoughts.

I am sorry if this seems to somehow step over your work, which is certainly good and well functioning.
I couldn't imagine I'd want to push a transform like this any further without playing with the code.
Ultimately I'm more than happy to receive feedback and collaborate on this.

@ntziolis
Copy link
Contributor Author

ntziolis commented Nov 4, 2021

@santino No apologies needed. I'm work & travelling until the 12th so my responses might be a bit slow.

I like the idea to extend the transfer to other parts of the schema. In fact after writing the transfer-field the only use case I could come up with that didn't overlap with hoist-field was moving fields from one root field to another (Mutation to Query etc.). Which felt a little light on usefulness.

I will have a closer at your code and especially at the unit tests you called out to understand where you want to take this. One thing that is front and center for me is that whatever configuration we come up with should be easy to understand and near self explanatory (similar or the other transforms).

I am sorry if this seems to somehow step over your work, which is certainly good and well functioning.
You never never need to worry about this with me, I'm purely goal focused which is is to jointly come up with the best solutions. Nothing else matters :)

@ntziolis
Copy link
Contributor Author

ntziolis commented Nov 4, 2021

@Lappihuan You should be able to use any other bare transform after using transfer-field. That said the intent is to "replace" the replace-field transform with smaller more focused transforms. The transfer-field transform being one of them. So once we have those smaller transforms in place santino suggested to retire depreceate replace-fields transform.

@ardatan ardatan force-pushed the master branch 2 times, most recently from 39e5048 to 7ef75ea Compare November 11, 2021 12:46
@santino santino mentioned this pull request Nov 17, 2021
10 tasks
@ntziolis
Copy link
Contributor Author

Closing, superseded by #3185

@ntziolis ntziolis closed this Nov 18, 2021
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