fix(store-devtools): replace direct with indirect eval
#4216
Conversation
`unwrapAction` internally runs `eval` if the argument is of type `string`. It is unclear why that `unwrapAction` was necessary in the first place, and it looks like an artifact from a older codebase. The line was added in 2018. Since `eval` is not best practice and according to ngrx#4213, esbuild has issues with it, it is removed.
✅ Deploy Preview for ngrx-io ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
The eval is probably there because you can dispatch an action via the DevTools. I assume this is just a string, and eval parses it to an action object.
IIRC manually dispatching actions didn't completely work with the NgRx global store.
I'll test/verify this later this week, if that's the case we can also resort to the alternatives mentioned in https://esbuild.github.io/content-types/#direct-eval
There was a problem hiding this comment.
My assumption was correct, via de DevTools you can dispatch an action.
This will be a string.
Because of this, I think we need to keep parsing the string using eval, but with the workaround mentioned in https://esbuild.github.io/content-types/#direct-eval
|
Got it, will do and also add a unit tests so that in the future it is clear why we require that kind of feature. |
This reverts commit 08f7b10.
use an indirect eval to avoid issues with esbuild: https://esbuild.github.io/content-types/#direct-eval
|
Okay, @timdeschryver, the updated version with tests has been pushed. |
|
@rainerhahnekamp is the PR title still relevant? I guess we should update it since |
rainerhahnekamp
changed the title
eval and unwrapActioneval
|
@markostanimirovic, you're right. I've renamed the title. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
From my perspective, this removes the unnecessary
evalmethod to get an action's payload.If somebody knows why the
evalwas there in the first place (no test is failing), we could provide a safer, alternative version ofunwrapAction.What is the current behavior?
Closes #4213
What is the new behavior?
The
payloadis directly used.Does this PR introduce a breaking change?
Other information