fix(runner): disable settings persistence temporarily and some fixes #8018
Conversation
ihexxa
changed the title
| const matchName = targetRequest.name.trim() === nextRequestIdOrName.trim(); | ||
| // find the last request with matched name in case multiple requests with same name in collection runner | ||
| const matchLastIndex = j === requests.slice().reverse().findIndex(req => req.name.trim() === nextRequestIdOrName.trim()); | ||
| const matchLastIndex = j === _.findLastIndex(requests, req => req.name.trim() === nextRequestIdOrName.trim()); |
There was a problem hiding this comment.
We aim to avoid the use of lodash for these operations. Was the previous function not working as intended?
There was a problem hiding this comment.
Yes, the previous function changes the pre previous behavior, the card contains more info, and linter complains of findlastindex. May i know why do we avoid lodash?
There was a problem hiding this comment.
Some reasons to avoid lodash:
- Most of it is not helpful and replaced by latest JS features
- Increases bundle size and makes us dependent on it. (In our case we include it for the pre/post scripts but shouldn't use it in the project)
- More things we need to learn in order to figure out how something works.
I'd suggest using the platform Array.findLastIndex and use a @ts-expect-error for it.
| const matchLastIndex = j === _.findLastIndex(requests, req => req.name.trim() === nextRequestIdOrName.trim()); | |
| // @ts-expect-error - Needs upgrade to latest typescript | |
| const matchLastIndex = j === requests.findLastIndex(req => req.name.trim() === nextRequestIdOrName.trim()); |
There was a problem hiding this comment.
There was a problem hiding this comment.
I have to say that my thought is a bit different: 🫤 :
lodashis a dependency of many modules, so it seems infeasible to totally exclude it from the bundle as it could be indirect dependency.- It also take time to learn the builtin approach, but I agree that a small part of lodash functions are not intuitive
- And this repo (18.7 stars) and lodash with 59.7 stars.
But im fine to go with the builtin approach as you mentioned more js features come into play.
| const [runnerAdvancedSettings, setAdvancedRunnerSettings] = useState<RunnerAdvancedSettings>(tempRunnerSettings.advanced); | ||
| const [isRunning, setIsRunning] = useState(false); | ||
|
|
||
| invariant(iterationCount, 'iterationCount should not be null'); |
There was a problem hiding this comment.
Since this is inside the render function what happens in the error case? If the iteration count can't be null maybe we should describe it better in the type instead of an invariant
There was a problem hiding this comment.
Currently the component checks this and rejects invalid values, include null. Agreed with that the invariant should be removed also.
jackkav
force-pushed
the
fix/disable-runner-cfg-persist
branch
from
53f4c85 to
eaaa8ee
Compare
| const matchName = targetRequest.name.trim() === nextRequestIdOrName.trim(); | ||
| // find the last request with matched name in case multiple requests with same name in collection runner | ||
| const matchLastIndex = j === _.findLastIndex(requests, req => req.name.trim() === nextRequestIdOrName.trim()); | ||
| const matchLastIndex = j === requests.slice().reverse().findIndex(req => req.name.trim() === nextRequestIdOrName.trim()); |
There was a problem hiding this comment.
@jackkav This is a bug fix (not style improvement) we should not revert it.
That's why I propose leaving a comment instead editing it in place, as the last setting persistence change has significantly introduced some issues.
Background
We plan to move forward without settings persistence temporarily and will bring back it later.
Changes
setNextRequestRequestRowshould not be an arrayRef: INS-4452