Conversation
|
Hey, Thank you for the PR. Libraries are not linked via pnpm workspaces because semantic-release doesn't allow to use pnpm instead of |
|
though, I'd like PR to be feature centric, so if it's about nested paths, then it should not contain all other infra related things. This should be done separately. I will do some updates to master |
|
I want to migrate to |
you are very welcome! |
Totally understand that and also wasn't my intension. I can revert the changes to the package.json regarding the version specification (workspace:*). |
I can also highly recommend https://github.com/googleapis/release-please and i have good experiences with it so far. Besides it works very well with monorepos. Besides, I would also be happy to help you in general with maintaining the project, if you need help. |
would be awesome. Thanks for the support! I just briefly looked into release-please but yeah the main idea is to have a release PR instead of releasing on every merge because then I can see the version change clearly and adjust it if it's wrong. |
|
I'd also replace @swc/core with esbuild because esbuild also fast and this project doesn't use decorators, so there is no much sense to use @swc/core. Moreover, by using esbuild we could get rid of rollup and other stuff. Distributing es5* versions also doesn't make sense nowadays |
|
or maybe tsup -> https://github.com/egoist/tsup |
|
I'll make a push to your branch to resolve conflicts after my changes in master. I upgraded eslint/typescript separately, so this change can be cleaner |
| type ArgsExceptLast<F extends (...args: any[]) => any> = | ||
| F extends (a: any, c: any) => any | ||
| ? Parameters<(condition: Condition) => 0> | ||
| : F extends (a: any, b: any, c: any) => any | ||
| ? Parameters<(condition: Condition, value: Parameters<F>[1]) => 0> | ||
| : Parameters<( | ||
| condition: Condition, | ||
| value: Parameters<F>[1], | ||
| options: Parameters<F>[2], | ||
| ...args: unknown[] | ||
| ) => 0>; |
There was a problem hiding this comment.
in the past, ts didn't support labels for tuples and in order to transfer proper argument names into inferred functions, I needed to use this construct Parameters<(condition: Condition, value: Parameters<F>[1]) => 0> because then this tuple preserverd the names in the function. without this it was just (arg_0: condition, arg_1: ....)
There was a problem hiding this comment.
I don't know how it works right now for this specific case but I'll revert this change to ensure it works as expected
There was a problem hiding this comment.
ah, ok, just checked. now it works even with spread operator
There was a problem hiding this comment.
no, actually not. Because this changes the inferred types. interpret function should have Condition as the first argument but after this change it infers it from the passed function which is wrong because sometimes it's CompoundCondition
There was a problem hiding this comment.
ArgsExceptLast is now a generic type for an arbitairy function and just strips the last Argument.
This should still work, because createInterpreter enforces that the passed function must extend AnyInterpreter, which is required to have Condition as the first argument.
There was a problem hiding this comment.
CompoundCondition extends Condition, so technically it satisfies the restriction but then it doesn’t allow to pass Condition to interpret nested conditions. It forces to pass CompoundCondition.
And as a result inside ConpoubdCondition, interpret helper cannot be used to evaluate nested conditions because ts fails
| @@ -50,29 +45,29 @@ export function createInterpreter<T extends AnyInterpreter, U extends {} = {}>( | |||
|
|
|||
| switch (options ? options.numberOfArguments : 0) { | |||
There was a problem hiding this comment.
I also don't think you have to make this differentiation by the number of arguments, whith my approach it works with arbitairy numer of arguments.
So an interpreter can have any amount of arguments and the context is still passed as last argument.
const interpret = ((condition : Condition, ...params: any[]) => {
const interpreterName = getInterpreterName(condition, options);
const interpretOperator = getInterpreter(interpreters, interpreterName);
return interpretOperator.apply(null, [condition, ...params, defaultContext]);// eslint-disable-line @typescript-eslint/no-use-before-define
}) as unknown as InterpretationContext<T>['interpret'];There was a problem hiding this comment.
Yeah, this is micro optimization. So we have a function with static amount of arguments. This is not used by end users anyway, so it's just for internal library composition
| type ArgsExceptLast<F extends (...args: any[]) => any> = | ||
| F extends (a: any, c: any) => any | ||
| ? Parameters<(condition: Condition) => 0> | ||
| : F extends (a: any, b: any, c: any) => any | ||
| ? Parameters<(condition: Condition, value: Parameters<F>[1]) => 0> | ||
| : Parameters<( | ||
| condition: Condition, | ||
| value: Parameters<F>[1], | ||
| options: Parameters<F>[2], | ||
| ...args: unknown[] | ||
| ) => 0>; |
There was a problem hiding this comment.
ArgsExceptLast is now a generic type for an arbitairy function and just strips the last Argument.
This should still work, because createInterpreter enforces that the passed function must extend AnyInterpreter, which is required to have Condition as the first argument.
|
I would also be happy to help you with the maintaining of the project.
I think you can achieve the same thing with both tools. Swc can also be used without rollup. |
great, than i will create a pull request for release-please. |
Just my 2 cents, you should try tsdown, the successor of tsup, based on rolldown |
3 participants
resolves #57
Besides implementing nested key paths, i also dumped the version of typescript from 3.x to 5.x (was 2 versions behind), as well replacing
babelwithswc, which lead to much faster builds.And for some reason you didn't link the packages between each other, which lead to pnpm pulling @swc/core from the npm registry instead of refering to the local workspace.