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 errors when using the monorepo #856

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Fix errors when using the monorepo #856

merged 4 commits into from
Jan 14, 2025

Conversation

josephjclark
Copy link
Collaborator

Short Description

When the vm throws an error, we try and determine if that error comes from an adaptor. This makes assumptions about the callstack and the path of the file. It works fine for production adaptors, but when loading from the monorepo these tests fail.

Fixes #855

QA Notes

I am reproducing like this:

  • In the monorepo, add this to common:
  • Create a simple job which just calls tmp()
  • From packages/cli, run pnpm openfn tmp/job.js -ma common (where tmp/job.js points to the simple job)
  • Against production, the error contains no details. Here, you get output like:
[CLI] ♦ Versions:
         ▸ node.js                    22.12.0
         ▸ cli                        1.10.0
         ▸ @openfn/language-common    monorepo
[CLI] ✔ Loading adaptors from monorepo at /home/joe/repo/openfn/adaptors
[CLI] ⚠ Skipping auto-install as monorepo is being used
[CLI] ✔ Compiled all expressions in workflow
[R/T] ✘ job-1 aborted with error (100ms)

[R/T] ✘ Error reported by "tmp()" operation line 1:
[R/T] ✘ Error: TEST_ERROR: this is a test
[R/T] ✘ Additional error details:
{
  type: 'Error',
  message: 'TEST_ERROR: this is a test',
  code: 'TEST_ERROR',
  description: 'this is a test',
  jam: 'jar'
}

[R/T] ✘ Check state.errors.job-1 for details
[CLI] ✔ State written to tmp/errors/output.json
[CLI] ⚠ Errors reported in 1 jobs
[CLI] ✔ Finished in 152ms

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

if (/(@openfn\/language-)|(vm:module)/.test(next)) {
if (
// detect an adaptor prod, adaptor monorepo, or vm frame
/(@openfn\/language-)|(adaptors\/packages\/.+\/dist)|(vm:module)/.test(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bit hairy

We're looking in the callstack or a path like adaptors/packages/common/dist

Because the path to an adaptor in the monorepo isn't terribly distinctive. path likepackages/*/dist feels like it might erroneously match against many paths (then again maybe not? Most node_modules code would be like packagename/dist, with nothing in between.

Anyway, because of this, I wanted to put the repo name in the path.

But that doesn't work for anyone who forks the repo with a different name locally. In those cases, the path won't match and bad errors will be triggered, which leads to misleading error reports when developing adaptors.

Perhaps I can do a regex like "not node_modules and packages/./dist"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah it's fine - I'm sure packages/./dist is all we need for this. There's only a small amount of non-vm error cases that we need to handle. Let's try it out.

@josephjclark josephjclark merged commit a1a071b into main Jan 14, 2025
10 checks passed
@josephjclark josephjclark deleted the monorepo-errors branch January 14, 2025 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Runtime: some error aren't logged at all!
1 participant