useReplicant contains stale values when using a different repName/namespace.
Closed this issue · 15 comments
Currently, swapping out the namespace using a prop does not cause useReplicant to use the new replicant name. My suspicion is that its because the internal useEffect never runs on subsequent re-renders.
Here's a gist where this problem manifests. The "OBSControlView" object always binds to the first namespace, even if the prop changes.
https://gist.github.com/CarlosFdez/ab3ab575a62c048daacb8f6b38a6f2d3
My suggestion is to add repName as a param to line 39. I wanted to ask here for the go-ahead before doing it.
Trying to test it locally so I cloned the project. Getting a cryptic error when I try to run the test. Using yarn and the newest version of node (11.14), adding an exception to engines so that yarn can install it. This happens even if I run build first
PS D:\Programming\Node\use-nodecg> yarn run test
yarn run v1.15.2
$ run-s format-test lint
$ yarn format --list-different
$ prettier "**/*.{js,ts,tsx,md,yml,yaml,json}" --list-different
.codecov.yml
.eslintrc.json
.prettierrc.js
.travis.yml
CHANGELOG.md
cjs\index.d.ts
cjs\index.js
cjs\useListenFor\index.d.ts
cjs\useListenFor\index.js
cjs\useReplicant\index.d.ts
cjs\useReplicant\index.js
cjs\useReplicantOnce\index.d.ts
cjs\useReplicantOnce\index.js
esm\index.d.ts
esm\index.js
esm\useListenFor\index.d.ts
esm\useListenFor\index.js
esm\useReplicant\index.d.ts
esm\useReplicant\index.js
esm\useReplicantOnce\index.d.ts
esm\useReplicantOnce\index.js
jest.config.js
README.md
src\__tests__\use-replicant.tsx
src\index.ts
src\useListenFor\index.ts
src\useReplicant\index.ts
src\useReplicantOnce\index.ts
tsconfig.cjs.json
tsconfig.esm.json
tsconfig.json
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "format-test" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Recently my impression is we probably should change the API to pass replicant instance instead of replicant name.
It allows more flexibility for declaring replicant, unless we duplicate the API in this library, which is not ideal. Also it allows reusing one replicant over different components.
@CarlosFdez @tedkulp I would be interested in what you think about the idea above.
In the case of this bug, I was able to fix it locally by adding replicant as a dependancy to useEffect, which is what the linter recommended I do anyways. Only problem is that I couldn't get the tests to work on my machine so no way I can submit a PR.
As far as that API change is concerned though, I'm fine with that, however I can imagine that being a bit verbose for the common case. Luckily, its pretty easy to detect if the first parameter is a string or if its a replicant, so I think its very possible to support both.
IMO my personal thought is that if breaking changes are not something we're concerned about, it should support both useReplicant(replicant) and useReplicant(name, replicantOptions).
Supporting both with the same hook would make more confusion than the benefit (of either) IMO. The way would be making another hook function.
As for the tests, the problem seems to be because of the lack of .prettierignore file. Would you be able to add that too?
Probably so, not particularly agreeing but that said I'm used to function overloading since nearly every language out there supports it, so its your call. That said replicantOptions is much more flexible than intialValue for the take a name case, as it also contains its own initial value.
Turns out that locally I solved it by using repName, not replicant. I'll have to see if nodecg caches Replicant instances cause otherwise it'll cause problems, especially if a version is made that takes a replicant directly.
What would I put in prettierignore? Never used prettier before. It seems that prettier is using a whitelist already (that is including md, yaml and json?)
For prettierignore, you can just copy paste the content of gitignore.
Didn't work, same issue just with less entries in the list.
PS D:\Programming\Node\use-nodecg> yarn run test
yarn run v1.15.2
$ run-s format-test lint
$ yarn format --list-different
$ prettier "**/*.{js,ts,tsx,md,yml,yaml,json}" --list-different
.codecov.yml
.eslintrc.json
.prettierrc.js
.travis.yml
CHANGELOG.md
jest.config.js
README.md
src\__tests__\use-replicant.tsx
src\index.ts
src\useListenFor\index.ts
src\useReplicant\index.ts
src\useReplicantOnce\index.ts
tsconfig.cjs.json
tsconfig.esm.json
tsconfig.json
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "format-test" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
When I swap to --check (did a bit of research), it says that those are code style issues, so it seems like the issue is just something regarding the current version. However it doesn't seem like npm run test actually runs any tests and I was probably led astray by a red herring.
I can look into it if you could submit the PR.
luckily npx jest works, its just that npm run test doesn't call it. There's a broken test in the current version so might as well try to fix it. Figured that out after I sent the previous message but there should be no problem.
I do have a few questions though. Currently I'm trying to fix the bug as its not decided what the new API will be. It also turns out that what I reported isn't exactly right. The problem isn't specifically with namespaces, its with replicant name prefixes. nodecg-obs calls it a namespace but its not actually a namespace, its just a replicant name prefix ('compositor:websocket transmission:websocket etc).
-
So NodeCG if you use the same replicant name it will return the same replicant instance. Do we emulate this behavior in the tests, or do I pass repName as a dependency in useEffect instead of replicant to sidestep everything? Either works.
-
General question, what's the recommended way of testing if a value is null OR undefined? Normally I use == null as it coalesces but the linter isn't liking that for this test. The answers I find on the internet are extremely verbose.
-
I don't get the contraries. Could you show some examples?
-
I'd prefer using
===always. If coalescing is preferred I would use!somethingthough you have to make sure you are OK to negate empty string and0too.
- If I use [replicant] as the dependancy in line 39 in useReplicant/index.ts, it'll work in an actual environment but fail the tests I added because your test always reuses the same replicant instance. In an actual nodecg environment, the same rep name returns the same replicant instance, changing it returns a new one.
However I just added eslint-plugin-react-hooks and it seems it wants me to use replicant regardless, so the question has been answered: mimic live functionality.
- Didn't work either (eslint no-negated-condition). This is the current code:
const RunnerName: React.FC<RunnerNameProps> = (props): JSX.Element => {
const { prefix } = props;
const repName = (!prefix) ? 'currentRun' : `${prefix}:currentRun`;
const [currentRun] = useReplicant(repName, {runner: {name: 'foo'}});
return <div>{currentRun.runner.name}</div>;
};
Checking for undefined is extremely annoying in javascript. Trying to do it Python style (check for falsy) but linter doesn't like it...
Just gonna solve 2 like this. I really don't like javascript's coalescing capability but unfortunately I can't change it: const repName = ${prefix || 'default'}:currentRun;. Sending a PR soon.