feat(types): resolves error types incompability with imt-blueprint #16

vokrik · 2024-10-17T11:34:54Z

https://make.atlassian.net/browse/CDM-12048

both proto and imt-bluperint override global error in incompatible ways. When both packages are included in a single project, you get a typescript error.

See comments inline for more explanation

make-coverage · 2024-10-17T11:34:57Z

Unit Test Coverage Report

Metric	master	CDM-12048-fix-types	Status
Statements	85.33%	85.54%	🌲
Branches	44.94%	46.15%	🌲
Functions	65.45%	66.07%	🌲

github-actions · 2024-10-17T11:35:17Z

	Messages
✅	Your PR title & description are valid

_{Generated 18. 10. 2024 12:55:04 GMT+2 for 5e72d2e}

vokrik · 2024-10-17T11:36:51Z

package.json

@@ -25,10 +25,12 @@
    "url": "https://github.com/integromat/imt-proto"
  },
  "license": "MIT",
+  "dependencies": {
+    "@types/request": "^2.48.12"


The request type is part of the interface of the Hook. By not including the type in the dependencies, you get typescript error. This issue haven't surfaced yet, because most of the repositories depend on something (like express) that bring in this dependency. But by using the package in imt-blueprint it surfaced

vokrik · 2024-10-17T11:43:14Z

src/error.ts

@@ -9,20 +9,23 @@ declare global {
    bundle?: Bundle;
    suberrors?: Error[];
    external?: any;
-
+    imtExceptionHash?: string; // moved from imt-blueprint - it is the same thing as hash, it is just a historical inconsistency
+    imtInternalError?: Error; // moved from imt-blueprint


Basically this is used just in blueprint I guess to distinguish distinguish: "file not loaded" from node and "Failed to load package" that is the actual intent. Imho it is a bad idea and if I was designing it, I would just translate the error to the "Failed to load package" and add the "file not loaded" to the "cause", but decided to keep it here for backwards compatibilty

vokrik · 2024-10-17T11:49:59Z

src/error.ts

-  toJSON() {
-    return Object.assign(super.toJSON(), this);
+  toJSON(): ErrorJSON {
+    return { ...this, ...super.toJSON() };


Imho this should be first and I would bet it caused unexpected behaviour on serialized properties. You called super.toJSON() that would for example serialize the suberrors, but then this object assign over them unserialized.

vokrik · 2024-10-17T11:52:02Z

src/error.ts

@@ -453,7 +468,22 @@ Object.defineProperty(Error.prototype, 'toJSON', {
    if (this.bundle != null) json.bundle = this.bundle;
    if (Array.isArray(this.suberrors)) json.suberrors = this.suberrors.map((item: Error) => item.toJSON());


I wonder if this is even necessary, cause I would expect the JSON.stringify() to actually go through all of the items in the array and call toJSON on them. I wonder why we are doing it manually here, but @skvelymake taught me not to touch such kind of things here :D

vokrik · 2024-10-17T11:53:34Z

src/global.ts

@@ -45,6 +45,7 @@ declare global {
  var IMTRPC: typeof publicApi.IMTRPC;
  var IMTFeeder: typeof publicApi.IMTFeeder;
  var IMTTransformer: typeof publicApi.IMTTransformer;
+  var IMTHITL: typeof publicApi.IMTHITL;


This was for some reason missing, but the blueprint has this line of code that depends on this in global:

Seems like we forgot to add it when doing some refactoring

if (klass.prototype instanceof IMTHITL)

vokrik · 2024-10-17T11:54:37Z

test/error.spec.ts

+        message: 'Type message.',
+        stack: ee.stack,
+      },
+      imtExceptionHash: 'im-hash',


Not super happy about repeating this one. I was thinking about not serializing it and keeping just the hash, but I guess something in datadog could depend on it for projects that assign this value.

vokrik · 2024-10-17T11:57:31Z

package.json

@@ -9,7 +9,7 @@
    "integromat",
    "imt"
  ],
-  "version": "2.4.0",
+  "version": "2.5.0",


Unless you see something here that would require a Major version bump. To me seems like minor

skvelymake

I like the changes.

Unfortunatelly I feel there will be more of these...

src/error.ts

skvelymake · 2024-10-18T09:25:34Z

package.json

@@ -9,7 +9,7 @@
    "integromat",
    "imt"
  ],
-  "version": "2.4.0",
+  "version": "2.5.0",


I tend to do version bump in a separate commit, but if you are going to release the library I think that's fine.

Co-authored-by: skvelymake <112627159+skvelymake@users.noreply.github.com>

feat(types): resolves error types incompability with imt-blueprint

fb8f3cc

vokrik requested a review from a team as a code owner October 17, 2024 11:34

vokrik commented Oct 17, 2024

View reviewed changes

Adds deprecation notice

f2bbf8b

vokrik force-pushed the CDM-12048-fix-types branch from 7f00e56 to f2bbf8b Compare October 17, 2024 11:46

vokrik commented Oct 17, 2024

View reviewed changes

bumps version

f1fa79b

vokrik force-pushed the CDM-12048-fix-types branch from cb05b04 to f1fa79b Compare October 17, 2024 11:56

vokrik commented Oct 17, 2024

View reviewed changes

makes imtExceptionHash configurable

61e798c

skvelymake approved these changes Oct 18, 2024

View reviewed changes

Update src/error.ts

5e72d2e

Co-authored-by: skvelymake <112627159+skvelymake@users.noreply.github.com>

vokrik merged commit 63bebd8 into master Oct 18, 2024
3 checks passed

vokrik deleted the CDM-12048-fix-types branch October 18, 2024 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(types): resolves error types incompability with imt-blueprint #16

feat(types): resolves error types incompability with imt-blueprint #16

vokrik commented Oct 17, 2024

make-coverage bot commented Oct 17, 2024

github-actions bot commented Oct 17, 2024 •

edited

Loading

vokrik Oct 17, 2024

vokrik Oct 17, 2024

vokrik Oct 17, 2024 •

edited

Loading

vokrik Oct 17, 2024

vokrik Oct 17, 2024

vokrik Oct 17, 2024

vokrik Oct 17, 2024

skvelymake left a comment

skvelymake Oct 18, 2024

		@@ -453,7 +468,22 @@ Object.defineProperty(Error.prototype, 'toJSON', {
		if (this.bundle != null) json.bundle = this.bundle;
		if (Array.isArray(this.suberrors)) json.suberrors = this.suberrors.map((item: Error) => item.toJSON());

feat(types): resolves error types incompability with imt-blueprint #16

feat(types): resolves error types incompability with imt-blueprint #16

Conversation

vokrik commented Oct 17, 2024

make-coverage bot commented Oct 17, 2024

Unit Test Coverage Report

github-actions bot commented Oct 17, 2024 • edited Loading

vokrik Oct 17, 2024

Choose a reason for hiding this comment

vokrik Oct 17, 2024

Choose a reason for hiding this comment

vokrik Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

vokrik Oct 17, 2024

Choose a reason for hiding this comment

vokrik Oct 17, 2024

Choose a reason for hiding this comment

vokrik Oct 17, 2024

Choose a reason for hiding this comment

vokrik Oct 17, 2024

Choose a reason for hiding this comment

skvelymake left a comment

Choose a reason for hiding this comment

skvelymake Oct 18, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2024 •

edited

Loading

vokrik Oct 17, 2024 •

edited

Loading