Closed SunPj closed 2 years ago
I think options
should be passed to mutate
method instead of destination
I have just compared this with rehype-remark which does opposite translation https://github.com/rehypejs/rehype-remark/blob/9.0.0/index.js
function (destination, options) {
return destination && 'run' in destination
? bridge(destination, options)
: mutate(destination)
}
The expectation I believe is that you either pass destination, options?
or options?
.
Hence, it should work with .use(remarkRehype, options)
@wooorm Thanks for clarification.
What would be a difference if we execute that passing only destination or options
.use(remarkRehype, options)
.use(remarkRehype, destination)
Will that work properly?
I’m not sure if I understand your question correctly?
This plugin does different things based on whether destination
is given.
But options
are also useful if a destination
is given. Hence both parameters are allowed
@wooorm Thanks. Passing options worked for me, but still it's not clear that we can omit destination and pass options. I think it is worth to add that case to readme or allow passing destination = null && options = {some value}
Yeah I’m open to such a change. Care to work on it?
@wooorm I would change mutate(destination)
to mutate(options)
so it's clear that we can execute this like mutate(null, options)
. I can create PR if you like it
That sounds like a braking change? I think this fixes your use case but otherwise keeps everything working the same as before:
-settings = destination || {}
+settings = destination || options || {}
And yes, PR welcome!
@wooorm It looks pretty confusing to me. The contract is that method expects two arguments F(a, b)
, it's totally fine to omit the second one. But when you call function with one argument F(y)
you expect that you execute F(a=y)
but not F(a=undefined, b = y)
. Said that with current function signature I pass options
instead of destination
and it magically works (because in function body we assume that destination is config if there is no "run" property in it, and we pass destination to mutate
method assuming that it is a config).
I suggest we keep this clear and remove tricky part.
I agree that introducing settings is good but anyway the proper method execution should look like F(null, options)
in my case.
Are you a TypeScript user?
Note that the documentation is .use(remarkRehype[, destination][, options])
. This implies to me that the second and third parameters can be omitted completely. And I mean omit as not passed at all, rather than set to a nullish value.
I believe what you are thinking of would be .use(remarkRehype, destination?, options?)
, which implies that both destination
and options
can be nullish.
I suggest we keep this clear and remove tricky part.
I’m not really interested in introducing a breaking change, across several projects (remark-rehype, remark-retext, rehype-remark, rehype-retext, ...) that breaks code that’s out there, for no good reason.
I agree that introducing settings is good but anyway the proper method execution should look like F(null, options) in my case.
I believe that that’s what my proposal allows. But your proposal breaks all existing cases.
I’m also open to adding a signatures section right above destination
in the readme. Perhaps that clears it up?:
###### Signatures
* `origin.use(remarkRehype, destination, options?)`
* `origin.use(remarkRehype, options?)`
@wooorm Sorry, I am not good at typescript. Give me some time to read through the docs. I will get back to this after. Thanks
Sorry, I am not good at typescript
No problem, not needed at all! I was just wondering if that’s where you were coming from. Usually TS users don’t like “overloads”, and these two function signatures is an instance of that, hence I was wondering if that was your background
Hi! This was closed. Team: If this was fixed, please add phase/solved
. Otherwise, please add one of the no/*
labels.
I just added support for an explicit destination: null
to solve your expectation!
Thanks @wooorm
Initial checklist
Affected packages and versions
^9.1.0
Link to runnable example
https://codesandbox.io/s/remark-rehype-debug-4cz8v?file=/src/index.js:488-532
Steps to reproduce
Pass a custom handler and see that it's not used
Expected behavior
Custom link handler should override default one
Actual behavior
Passed handlers are ignored
Runtime
Node v16
Package manager
npm v6
OS
Linux
Build and bundle tools
Webpack