search

paritytech / opstooling-js

Shared functionality for OpsTooling projects
https://www.npmjs.com/package/@eng-automation/js
Apache License 2.0
0 stars 1 forks source link

Proposal: Replace Ok and Err classes with interfaces #35

Closed rzadp closed 1 year ago

rzadp commented 1 year ago

The (potential) problem

I worry that relying on instanceof Ok/Err in packages consuming opstooling-js can be dangerous.

Example usage

It is safe if the class is declared in your code, but if the class is declared in a dependency I think it's not always safe.

Multiple opstooling-js in node_modules

yarn add  --network-concurrency 1 --dev https://github.com/paritytech/opstooling-js#v0.0.16
yarn add  --network-concurrency 1 --dev https://github.com/paritytech/opstooling-testing

That's it, there now exist two different opstooling-js packages in node_modules. Can we be sure that when doing something instanceof Err both left-side value and right-side type are coming from the same package? I'm not so sure, I've run into problems like this before, but maybe I'm missing something.

The proposed solution

Replace the Ok/Err classes with interfaces. Now you cannot do result instanceof Ok, you have to do result.kind === "Ok", which I think resolves this potential problem.

mutantcornholio commented 1 year ago

I'd consider deprecating those, actually.

This is a practice coming from the Rust world (and in Rust, I love it), but it goes against common JS/TS practices, where throws and try-catches are standard.

WDYT?

Bullrich commented 1 year ago

I'd consider deprecating those, actually.

I agree here, if they don't provide that much use and are a danger, let's deprecate them and later remove them

rzadp commented 1 year ago

Ok, I have created this instead then: https://github.com/paritytech/opstooling-js/pull/36