-
Notifications
You must be signed in to change notification settings - Fork 22
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
Draft: Breaking: assoc, dissoc, and modify #37
base: develop
Are you sure you want to change the base?
Conversation
b529d67
to
1ad6ae3
Compare
@Nicholaiii @kedashoe @adispring Please review |
I like it, but ya I do imagine this would break some things for people using ramda + typescript. Also a bit of a shame Are |
In modern typescript, plan objects generally serve as one of two thing: Structs or Maps; The typing updates that I did here are to support both. With the Struct variety, you have the object shape defined ahead of time and are never really adding properties that wouldn't be defined. The other, Map, is I've been writing typescript almost exclusively for 5 years now and in my experience, these type updates are NOT likely to break anyone's typescript code. And it's because of how the updates mimic typescript behavior for when you directly mutate an object. It is still "Breaking" behavior for the function type definitions, but I truly believe the impact will be low
type Foo = {
foo: string;
};
const foo: Foo = { foo: '' };
assoc('bar', 'value', foo); // type-error, `'bar'` does not exist on type Foo
addProp('bar', 'value', foo); // ok, returns type `Foo & { bar: string }`
// alternatively, you can upcast to what the new type will be
type FooBar = Foo & { bar: string };
assoc('bar', 'value', foo as FooBar); // ok, returns type `FooBar` |
Ah I think I did not fully understand this, ty. Even though (as I think you are getting at) they have the same underlying representation in JS, they are different operations on different types. imho having 👍 |
I'm thinking of throwing this MR up in a few subreddits to pull in community opinions |
@person1123 @y-a-v-a @larssn @Morgantheplant @joeyquarters @theres-jon @kiandinyari @ACrystalC @adrogon @valentinpalkovic Tagging all of you since you participated in #64. I wanted everyone to be aware of and ask for feedback to this MR |
@tacomanator @akshayjai1 @klepek42 @char0n tagging you all for your participation on ramda/ramda#3415. Would like your opinion on this |
@Harris-Miller not competent enough in TypeScript to give opinion here. |
@Harris-Miller Happy to see the change. My opinion is considerably less qualified, but as I understand it builds will break and, in some cases, alternatives need to be swapped in. For this reason I personally would prefer a version bump. |
At first glance I find this to be a bit restricting. I find the the pattern of gradually assoc'ing new props to objects as part of a data transformation pipeline quite useful and natural. |
Could you private concrete example using https://www.typescriptlang.org/play? |
This is a common FP approach. Here's fp-ts with |
@fuadsaud @Nicholaiii const mainDo: T.Task<{ x: string; y: string }> = pipe(
T.Do,
T.bind('x', () => readLine),
T.bind('y', () => readLine),
T.tap(({ x }) => print(x)),
T.tap(({ y }) => print(y))
) The way that the generic is set on
The same should go for Just as A contrived example: // acceptable now, would error after change
const fn1 = pipe<[{}]>(
assoc('x', 'foo'),
assoc('y', 'bar'),
); // (arg: {}) => { x: string, y: string }
fn1({}); // { x: string, y: string }
// acceptable after change
const fn2 = pipe<{ x: string, y: string }>(
assoc('x', 'foo'),
assoc('y', 'bar'),
); // (arg: { x: string, y: string }) => { x: string, y: string }
fn2({} as { x: string, y: string }); // { x: string, y: string }
// Using `Record<>` would also work as expected both now and after change
const fn3 = pipe<Record<string, string>>(
assoc('x', 'foo'),
assoc('y', 'bar'),
); // (arg: Record<string, string>) => Record<string, string>
fn3({}); // Record<string, String> As stated in the description, I'm looking to solve a via typings a fundamental difference between javascript and typescript. While you can on any object in javascript Yes, javascript is a dynamic language, but is inherently unsafe as a result. Typescript does its best to add a layer of type safety, and I believe matching the expected behaviors of Another way to look at it is the signatures for these functions are different between javascript and typescript
|
There was a problem hiding this 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 positive change, and for a couple of reasons. First, the logic is flawed.
With this basis:
type Obj = {
str: string;
num: number;
};
const obj: Obj = { str: 'foo', num: 1 };
assoc
is not equal to following unsafe operation:
obj.foo = 'bar'
but rather it is equivalent to this typesafe operation:
const obj2 = {
...obj,
foo: 'bar'
}
It does not assign but associate, on a new, cloned object.
With these changes we are unable to do type transformation pipelines with assoc, modify or dissoc and I think that's really unhelpful. We are going to have to do either typecasting (with as
) or similar much less typesafe code to compensate.
I think finally it's worth noting that the types that are output by assoc, dissoc and modify are all very typesafe already. They do not output the input type, but a modified type that reflects the changes. This is by far the most flexible, useful and true-to-the-basecode option.
(on a side-note: I think something in pipe or compose is breaking types. It seems to strip the original type and only output the modification of these)
The first |
Today while writing some code following this same pattern I tried using lenses to update some values in an object (changing type) and, to my surprise, it seems to behave as proposed by this PR. Using spread works fine. type ExternalData = {
id: string,
createdAt: string
}
type InternalData = {
id: string,
createdAt: Date
}
function internalizeWithSpread(data: ExternalData) {
return {data, createdAt: new Date(data.createdAt)}
}
function internalizeWithLens(data: ExternalData) {
return R.over(R.lensPath('createdAt'), (d) => new Date(d), data)
)
}
function internalizeWithAssoc(data: ExternalData) {
return R.assoc('createdAt', new Date(data.createdAt), data)
}
I agree that
Yes. An object can evolve over time and be transformed into new things with different shapes. I don't see a reason why evolution should be restricted to |
I believe this to be half-true. Let me run you both through my thought processes here. Let's start with a type and a function: type Temp = {
scale: string;
value: number;
}
function isFreezing(temp: { scale: string; value: number}) {
let rtn = false;
switch (temp.scale) {
case 'celsius': rtn = temp.value <= 0;
case 'farenheit': rtn = temp.value <= 32;
case 'kelvin': rtn = temp.value <= 273.1;
// no default, assume exhaustive
}
return rtn;
}
const original: Temp = {
scale: 'fahrenheit',
value: 15
};
const updated = R.dissoc('value', original);
// ^? Omit<Temp, 'value'>
isFreezing(updated); // error ! However, they are not when it comes to their arguments function toC(temp: Temp) {
if (temp.scale == 'fahrenheit') {
// misspelled 'scale'! no warning from typescript
const next = R.assoc('scal', 'celsius', temp);
return R.modify('value', t => (t - 32) * 5/9, next);
}
if (temp.scale == 'kelvin') {
const next = R.assoc('scale', 'celsius', temp);
return R.modify('value', t => t - 273.15, next);
}
// already celsius, return input
return temp;
}
const toCheck = toC(original);
// ^? Temp & Record<'valeu', string>
isFreezing(toCheck); // no error! `Temp & Record<'valeu', string>` satisfies `Temp`
// -> The imperative mutable equivalent catches the misspelling function mutToC(temp: Temp) {
if (temp.scale == 'fahrenheit') {
temp.scale = 'celsius';
temp.value = (temp.valeu - 32) * 5/9; // misspelling caught by type checker
return temp;
}
if (temp.scale == 'kelvin') {
temp.scale = 'celsius';
temp.value = temp.value - 273.15;
return temp;
}
// already celsius, return input
return temp;
} The changes in this MR would fix the issue above. And weighing this case against the transformation pipeline use case, I believe that this holds more value for the typing of these functions for function internalizeWithAssoc(data: ExternalData): InternalData {
// required to cast here is more verbose,
// but it has the benefit of also showing the intention of changing the type from `ExternalData` to `InternalData`
return R.assoc('createdAt', new Date(data.createdAt), data as InternalData)
}
// IMO though, `evolve` would be a more appropriate function for this operation in general
function internalizeWithEvolve(data: ExternalData): InternalData {
return R.evolve({ createdAt: (x: string) => new Date(x) }, data);
}; While it's true that you can also do this with Lenses, the fact that you can is simply due to javascript dynamic types. It really doesn't mean that you should. It also does not match the type for it function internalizeWithLens(data: ExternalData) {
// The type for `over` is `Lens s a → (a → a) → s → s`
// however this operation is `Lens s a → (a → b) → s → Omit<s, ...> & Record<..., b>
return R.over(R.lensPath('createdAt'), (d) => new Date(d), data)
)
}
// same for R.modify
function internalizeWithModify(data: ExternalData) {
// The type for `modify` is `Idx → (v → v) → {k: v} → {k: v}`
// however this operation is `Idx → (v → z) → {k: v} → {k: z}`
return R.modify('createdAt', (d) => new Date(d), data)
)
} IMO, using
R.assoc('c', 3, {a: 1, b: 2}); //=> {a: 1, b: 2, c: 3} Expanding this out a bit give you const original = {a: 1, b: 2};
const next = R.assoc('c', 3, original); //=> {a: 1, b: 2, c: 3} Adding types type Obj = { a : number; b: number; c?: number }; // c? optional
const original: Obj = {a: 1, b: 2};
const next = R.assoc('c', 3, original); //=> {a: 1, b: 2, c: 3} It's not that I believe Assignment vs AssociationYes, this is true. We can go back and forth on this all we want. Let's agree to disagree. This MR is 8 months old now. I haven't merged it because of how Breaking it is. And I also never got a chance to follow through with the greater community on this. We are only 3 people, and I've personally never used Thank you |
Can you provide an example of this? |
There are a lot of points to address here, but let me start with the first: you misunderstand the fp-ts example if you think it backs your case up. I understand the plea for your intermediate usecases, and I both admire and respect that you didn't merge, but I think maybe a new function (eg |
|
I was hit by a similar situation yet another time 😅 : captureException(error, { extra: dissoc('error', object) }) In this code, object is a generic object that could contain any keys. The only requirement I have is: I want to transform object so that it doesn't contain an error key. I could do something like |
This case would still be covered by my proposed changes if type Obj = {
foo: string;
bar: number;
error?: Error;
}
|
@Harris-Miller what if you don't control the type of object? In my particular case, object comes from a third-party library (pino). It is, by design, an open object that could contain any key of any type (like an open record). Semantically, I'm accepting an open record, don't want to make any judgements about it, and just want to make sure that I'm passing it forward without a certain key. Of course I could narrow the type down and do some acrobatics to conform (e.g. check if prop is in the object), but that again increases the verbosity of the code without any particular real gain. And this is actually the behavior already observed in the released version of types/ramda with dissoc/omit. Especially with R.omit, it feels natural that it should map to TS's |
With this MR, const rec: Record<string, number> = {};
// all of these are ok
delete rec.foo;
delete rec.bar;
// therefor, so are these
dissoc('foo', rec);
dissoc('bar', rec); The type Note that for an object typed as |
@Harris-Miller got it. The case I mentioned last also works for |
Nevermind, found and fixed the issue: #100 |
Yes please do fill out a separate issue As a general note about javascript objects and typescript (well, my opinion, really). We generally use them as either a hash or as Before the native Map existed, we just objects for this case, which can be best represented in typescript by While in a lot of cases, The reason why I bring this up is while the hash usage is more of a struct, the map usage is more of a container. And the const strColl: Record<string, string> = {};
const intColl: Record<string, number> = R.map(x => Number.parseInt(x, 10), coll); ^^ This is just A plain-object serving as a hash, is not a Functor (eg like your It makes it really hard to type functions that except objects are arguments when it's near impossible to tell what the use case is going to be and how users intend to use the functions provided by |
Remove blanket support for object shape alteration
The previous type definition for
assoc
allowed any key to be added to any object. It also allowed for the value type of a key to be changed. The new definition strictly adheres to the type of object passedThe reasoning behind this is simple: Type Safety. Neither of those 2 operations would be allowed when mutating the object directly
And typings for
assoc
should not either.Similar behavior can be seen with
dissoc
. The current definition allows you to remove a key from an object without issues. The new definition forbids that unless that key is marked as optional or| undefined
This is to match the behavior of the
delete
operation, whichdissoc
uses under-the-hood.Note that
| null
is not allowed either. That is intention by bothdelete
anddissoc
.Record<string, number>
type definitions are the exception here since all keys are technically optionalI do consider this a Breaking change, because it can/should break existing usage. However, I do believe it is acceptable to do without bumping both
ramda
,@types/ramda
, andtypes-ramda
to0.30.0
because it is really fixing the behavior more than anything elseIf in typescript you are still looking to be able to add properties onto an object where they are not defined on said object,
mergeLeft
ormergeRight
would be better suited for this task as that is more about creating new shapesSimilarly for
dissoc
,omit
can be used to create a shape without the property you want