Skip to content

common: move field() to util and remove export of common's field in adaptors #1173

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

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

Conversation

hunterachieng
Copy link
Contributor

@hunterachieng hunterachieng commented Apr 30, 2025

Summary

Move field() function to util in common

Fixes #1118

Details

Add technical details of what you've changed (and why).

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 ?
  • If this PR includes breaking changes, do we need to update any jobs in
    production? Is it safe to release?
  • 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?

hunterachieng and others added 23 commits April 24, 2025 18:28
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
* feat: update rapidpro

Signed-off-by: Hunter Achieng <[email protected]>

* feat: add changeset

Signed-off-by: Hunter Achieng <[email protected]>

* fix: remove config and fix r`return await`

Signed-off-by: Hunter Achieng <[email protected]>

* fix: update changeset

Signed-off-by: Hunter Achieng <[email protected]>

* fix: remove export of http

Signed-off-by: Hunter Achieng <[email protected]>

* fix: update changeset with removal of http export

Signed-off-by: Hunter Achieng <[email protected]>

* fix: remove nock

Signed-off-by: Hunter Achieng <[email protected]>

---------

Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
…1171)

* feat: use common-request

Signed-off-by: Hunter Achieng <[email protected]>

* feat: add changeset

Signed-off-by: Hunter Achieng <[email protected]>

* fix: update changeset

Signed-off-by: Hunter Achieng <[email protected]>

* fix: update changeset

Signed-off-by: Hunter Achieng <[email protected]>

---------

Signed-off-by: Hunter Achieng <[email protected]>
…ptors into feature/1120-common-migration

Signed-off-by: Hunter Achieng <[email protected]>
common: remove deprecated expandreferences and update common versions
Signed-off-by: Hunter Achieng <[email protected]>
@hunterachieng hunterachieng changed the base branch from main to epic/common-3 April 30, 2025 08:31
Signed-off-by: Hunter Achieng <[email protected]>
@@ -195,4 +196,15 @@ export {
* console.log(id); // Output:'3f4e254e-8f6f-4f8b-9651-1c1c262cc83f'
*/
uuid,
/**
* Returns a key, value pair in an array.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the blocking problem: we cannot manually duplicate these docs across all adaptors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These docs need removing :)

* remove common.map

* changeset

* typo

* remove map from salesforce and mogli

* changesets
Signed-off-by: Hunter Achieng <[email protected]>
@hunterachieng hunterachieng requested a review from josephjclark May 5, 2025 16:14
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.

Hi @hunterachieng - I've had a careful look through this and spotted a couple of things.

This is going to be such a huge piece of work 🤦 I'm going to do some user research before we commit more time to this. I don't want to do it if it's not valuable (I think it's a big improvement, but if I'm the only who thinks that then what's the point?)

Based on the time it's taken to do field, can you estimate how long it will take to move the other 10 (I think?) util functions?

@@ -298,9 +298,7 @@ export {
fn,
fnIf,
each,
http,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was removing http here deliberate? Also seen in odoo, so maybe there are others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We did remove the old http implementation in common, do we need to keep exporting it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, well yes you're right. But that's not mentioned anywhere in the PR title or description so it surprised me. And I thought that stuff had been done on the epic.

We need to make sure that this is recorded in a changeset because it's super important to document. Maybe you can generate a major changeset for this now?

We'll also need a major changset for every change to field - but maybe we don't need to do that one yet. I'm not sure the easiest way to handle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ShouldI go through every adaptor and remove the exportation on http and create a major changeset for each?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hang on, hang on, there's some confusion here.

When we removed http from rapidpro, we did so because it was exporting the old http namespace. It was version locked to common v1, and we didn't want to export the common v1 http namespace anymore.

That was actually wrong to do - we forgot that we'd updated to the latest common, so we should have left rapidpro to export http. Probably. But don't worry about that for now, I don't want to spend time on rapidpro.

But for other adaptors, we definitely don't want to change common http exports. Not here in this PR. We should not be touching mysql's http export here. If we want to do that, it must be on a new branch.

So for now, please revert all changes to http exports on this branch. We'll look at the in isolation as a separate exercise.

And in future, please try to avoid making unrelated changes in a PR! Try to stay focused on the task in hand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will improve on that. I have pushed the fixes

@hunterachieng
Copy link
Contributor Author

hunterachieng commented May 6, 2025 via email

@hunterachieng hunterachieng changed the title common: move field() to util common: move field() to util and remove export of common's field and http in adaptors May 6, 2025
@hunterachieng hunterachieng changed the title common: move field() to util and remove export of common's field and http in adaptors common: move field() to util and remove export of common's field in adaptors May 7, 2025
@hunterachieng hunterachieng requested a review from josephjclark May 7, 2025 08:14
@hunterachieng
Copy link
Contributor Author

@josephjclark about this PR. Should we still continue with the exports?

Base automatically changed from epic/common-3 to main July 10, 2025 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

common: move non-operations into util namespace
2 participants