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

Common add as() function #920

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PiusKariuki
Copy link
Collaborator

Summary

Performs and operation and saves the result in a pre-defined key while preserving any values in the data key.

Fixes #800

Details

I save the initial state data value in a variable then after I perform the operation I return that value bak in the data key in state.

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, added the adaptor on marketing website ?
  • Are there any unit tests?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.
  • Have you ticked a box under AI Usage?

…n value to a pre-defined key in state while preserving any values in the `data` key
@PiusKariuki
Copy link
Collaborator Author

@josephjclark What do you think about this?

@josephjclark
Copy link
Collaborator

@PiusKariuki That's actually really nice - as('lookup_table.option_set', get('fixture/?fixture_type=option_set_mapping')); reads really nicely.

This has been sitting at the back of my mind for a while and I didn't think it would work well. But seeing it here I love it!

Let me circulate for some feedback within the team. Thank you!

Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

Some comments but I'd like to sleep on this and speak to the team before committing it to the public!



/**
* Runs an operation and saves the return value to a pre-defined key in state
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, almost!

The ask here is to save what would have been saved to state.data onto state.<key>

I'll pick this up in comments on the test


const { foo: {data: fooData} } = await as('foo', mockOperation('https://localhost:1/as'))(state);

expect(fooData).to.eql('this data object has been changed');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually expect this to be more like:

const result = await as('foo', () => ({ data: 'bar' }))(state);
expect(result).to.eql({ foo: 'bar' });

* @public
* @function
* @example
* as('lookup_table.option_set', get('fixture/?fixture_type=option_set_mapping'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This example is interesting. If the key has dot notation, should it be a path? This would be easy to implement with lodash.set. But I also don't know the use-case

* @function
* @example
* as('lookup_table.option_set', get('fixture/?fixture_type=option_set_mapping'));
* @param {string} key - The key used to store the return value in the state object
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note for me: Perhaps as well as the string, we can accept an object which maps stuff. So "key" and { data: "key" } would be equivalent.

This would bypass the problems of a) only state.data gets mapped and state.data is kind of arbitrary, and b) what if I want to map multiple state keys, like http data AND response?

Please don't implement this yet, it might be too complicated

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.

common: add something like a save() or as() function
2 participants