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 broken commit #5219

Merged
merged 1 commit into from
Jun 7, 2024
Merged

fix broken commit #5219

merged 1 commit into from
Jun 7, 2024

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Jun 3, 2024

accidentally introduced here 2f21ce9

note the documentation here which correctly describes how it should work

Copy link
Contributor

github-actions bot commented Jun 3, 2024

Diff output files
diff --git a/packages/@uppy/companion/lib/server/Uploader.js b/packages/@uppy/companion/lib/server/Uploader.js
index a391f1f..da46ef5 100644
--- a/packages/@uppy/companion/lib/server/Uploader.js
+++ b/packages/@uppy/companion/lib/server/Uploader.js
@@ -421,13 +421,15 @@ class Uploader {
    */
   async #emitError(err) {
     // delete stack to avoid sending server info to client
+    // also remove extraData from inside the error object (it is provided outside)
     // see PR discussion https://github.com/transloadit/uppy/pull/3832
-    // @ts-ignore
     const { serializeError } = await import("serialize-error");
-    const { stack, ...serializedErr } = serializeError(err);
+    // @ts-ignore
+    const { stack, extraData, ...serializedErr } = serializeError(err);
     const dataToEmit = {
       action: "error",
-      payload: { error: serializedErr },
+      // @ts-ignore
+      payload: { ...extraData, error: serializedErr },
     };
     this.saveState(dataToEmit);
     emitter().emit(this.token, dataToEmit);

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I don't think this is a good change, we had a TODO for removing this, and this is just adding it back:

// todo remove also extraData from serializedErr in next major,

In any case, we should probably have a test for this, as this is a bit nebulous if we don't know what's the use case.

@mifi
Copy link
Contributor Author

mifi commented Jun 3, 2024

I don't think this is a good change, we had a TODO for removing this, and this is just adding it back:

extraData is still present in payload.error in the 4.x branch. My original PR removed it from payload.error (because it is already spread in payload). there was never any wish to remove it completely, but to de-duplicate it.

This PR replicates my original PR by removing extraData from payload.error which is what was originally intended. see discussion in #3832 (comment)

also our migration guide describes this:

The Companion error event now no longer includes extraData inside the payload.error property. extraData is (and was also before) included in the payload.

  • if you agree to keep it, then i can write a test for it.
  • if we don't want extraData to be included at all in the error event, then we should remove it from payload.error too.

@aduh95
Copy link
Contributor

aduh95 commented Jun 4, 2024

The thing is, this thing is undocumented (well except now in the migration guide that you authored) and untested. From what I understand in the TODO, we wanted to remove the special-casing of extraData that we don't use – so it would still be found in the error property, and no longer in the object. Now I understand it can be understand the other way around (like in this PR), which I didn't see until you explained.
I still think we should go with the simpler approach where we don't special-case extraData until someone comes up with a use-case – at which point we should add a test and document it.

@mifi
Copy link
Contributor Author

mifi commented Jun 4, 2024

ok, I agree that we can remove extraData because it's not documented anywhere.

I tried to remove extraData in 51b21a9 but as far as I understand, gotobusstop thought it was being used in (old versions of) Uppy: #3832 (review) so we kept it and added the TODO to remove it.

but if you think it's safe to remove it now, i'm not going to oppose that and we can close this PR

It's not an actionable item, and that change is unlikely to break
anyone anyway.
@aduh95
Copy link
Contributor

aduh95 commented Jun 7, 2024

Let's remove the now wrong note from the migration guide – after all, it's not an actionable item, and we don't expect that change to break anyone.

@aduh95 aduh95 merged commit d1094e1 into 4.x Jun 7, 2024
5 checks passed
@aduh95 aduh95 deleted the extradata-fix branch June 7, 2024 14:06
github-actions bot added a commit that referenced this pull request Jun 13, 2024
| Package              |       Version | Package              |       Version |
| -------------------- | ------------- | -------------------- | ------------- |
| @uppy/aws-s3         |  4.0.0-beta.6 | @uppy/react          |  4.0.0-beta.6 |
| @uppy/locales        |  4.0.0-beta.3 | @uppy/transloadit    |  4.0.0-beta.8 |
| @uppy/provider-views |  4.0.0-beta.8 | uppy                 | 4.0.0-beta.11 |

- docs: clarify assemblyOptions for @uppy/transloadit (Merlijn Vos / #5226)
- @uppy/react: remove `react:` prefix from `id` & allow `id` as a prop (Merlijn Vos / #5228)
- docs: correct allowedMetaFields (Merlijn Vos / #5227)
- docs: remove `extraData` note from migration guide (Mikael Finstad / #5219)
- meta: fix AWS test suite (Antoine du Hamel / #5229)




| Package              | Version | Package              | Version |
| -------------------- | ------- | -------------------- | ------- |
| @uppy/locales        |   3.5.4 | @uppy/transloadit    |   3.7.1 |
| @uppy/provider-views |  3.12.1 | uppy                 |  3.26.1 |

- meta: Improve aws-node example readme (Artur Paikin / #4753)
- @uppy/locales: Added translation string (it_IT) (Samuel / #5237)
- @uppy/transloadit: fix transloadit:result event (Merlijn Vos / #5231)
- @uppy/provider-views: fix wrong font for files (Merlijn Vos / #5234)
Murderlon added a commit that referenced this pull request Jun 17, 2024
* 4.x:
  Renames & `eslint-disable react/require-default-props` removal (#5251)
  coalesce options `bucket` and `getKey` (#5169)
  @uppy/aws-s3: add `endpoint` option (#5173)
  @uppy/locales: fix `fa_IR` export (#5241)
  improve companion logging (#5250)
  Release: [email protected] (#5243)
  @uppy/core: add generic to `getPlugin` (#5247)
  docs: add 4.x migration guide (#5206)
  @uppy/transloadit: also fix outdated assembly transloadit:result (#5246)
  docs - fix typo in the url
  @uppy/core: set default for Body generic (#5244)
  Release: [email protected] (#5242)
  docs: clarify assemblyOptions for @uppy/transloadit (#5226)
  meta: Improve aws-node example readme (#4753)
  @uppy/react: remove `react:` prefix from `id` & allow `id` as a prop (#5228)
  Added translation string (it_IT) (#5237)
  docs: correct allowedMetaFields (#5227)
  @uppy/transloadit: fix transloadit:result event (#5231)
  docs: remove `extraData` note from migration guide (#5219)
  @uppy/provider-views: fix wrong font for files (#5234)
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.

2 participants