feat: withDevTools disabled in prod - tree-shaking docs

[marcindz88]

Solution to #53

[rainerhahnekamp]

Thanks @marcindz88. In the meantime, I've looked up the approach of the NgRx Devtools.

The approach for tree-shaking is very similar to what you proposed, namely, using the environment files.

Disabling it with isDevMode() does not exist. Instead, the user can set a logOnly property, which disables certain features like time traveling.

What if we provide logOnly as an option that is enabled by default instead?

Time traveling is unavailable at the moment, so it would not have an immediate effect.

Users could use the extension like

withDevtools('flights': options: {logOnly: !isDevMode()})?

And the signature could be

type Options = {
  logOnly: boolean
}
withDevtools(name: string, options: Options = {logOnly: false);

[marcindz88]

@rainerhahnekamp Yeah I was thinking, that some users may want to enable devtools in some prod-optimized environments like test staging. However I don't really like repeating the configuration in every store in app.

Does this feature work in injection context? If yes maybe we could provide a provider that creates an injection token with global ngrx-toolkit configuration?

[rainerhahnekamp]

@marcindz88

However I don't really like repeating the configuration in every store in app.

You are right, I am all for an "injection" token approach. This would also fix the issue with one store having logOnly set to to true and another to false.

withDevtools has access to the injection context and as soon as it becomes relevant, i.e. it supports time travel, it can request the value of the injection token.

---

Btw, why do you disable dev tools in a production environment? I always found it extremely helpful for debugging and reproducing issues that only occur in production.

It is not that we are exposing any secrets. Anyone can monitor the network requests and—with a little bit of work—find out what's going on by looking at the minimized Javascript files.

Just curious to learn something new here.

[marcindz88]

I think there are several arguments for disabling / removing it in production:

1. Performance - Redux monitoring tool can become an overhead for application, as it stores the state and history of states of the application. I have seen it lag in large stores.
2. Bundle size
3. While a user can always find everything in obfuscated javascript files, it requires very good knowledge of Angular and is still non-trivial to observe actions and the flow of application state. It is hard to find and see the state of application without using debugger and knowledge about the app structure. With the Redux extension everything is visible for anyone, who has installed it, without much effort.

Do you think we should add one configuration token for the whole library or a per-feature approach?

[rainerhahnekamp]

Thanks for the detailed explanation. I suggest we go with one global configuration token. It should be just on or off

[marcindz88]

@rainerhahnekamp please take a look.

I have observed while testing that the store does not disconnect after application restart, I think this is because connect observer is not unsubscribed nor is disconnect called anywhere FYI https://github.com/reduxjs/redux-devtools/blob/main/extension/docs/API/Methods.md#disconnect.

Also, I was thinking and we could use the pattern of overriding action creators to be able to detect action dispatches without the use of updateState.

[marcindz88]

@rainerhahnekamp please also take a look at this experiment: 1c7ee5d
It simplifies the code a bit and now whole actions with payload are visible in redux dev tools with no need for overriding of patchState.
However there is an issue with private actions - because these are not methods in the store they are not visible, but we could probably work around this somehow. Also the new dispatch action approach is probably not compatible, but we could override dispatch fn with the dev tools logic.

[rainerhahnekamp]

@marcindz88 thanks, I have another branch that deals with disconnection issues and some other stuff but haven't found the time to push it yet.

I'll also checkout the automatic action name detection change, although I still think updatePatch has its use case when developers want to provide a specific, readable name for the action.

[rainerhahnekamp]

Good work so far

[rainerhahnekamp]

logOnly doesn't have an effect at the moment. Logging still happens but features like time travelling will be disabled.

Since we "only" have logging at the moment, we can leave this condition with isServer only.

[marcindz88]

I don't know if we should include this config for now. Maybe let's stash it for later (when it will do something) and merge only the tree-shaking docs ?

[rainerhahnekamp]

I am fine with merging only the docs

[rainerhahnekamp]

Do we want to make that as an environment provider so that it could only be called in the root scope or in paths?

What about override protection? I think that would also be useful.

[marcindz88]

@rainerhahnekamp regarding my experiment version: It is not only about the actionName, we should also pass action payload to redux extension. Also, we could prefix the action name with [name of store] and possibly change camelcase to a more readable version.

[rainerhahnekamp]

@marcindz88 In order to get only the documentation part merged, could you please remove all files except the README from your PR?

[marcindz88]

@rainerhahnekamp PR updated, I also added an a stub const for easier use in the environment file. Let me know if that's ok.

[rainerhahnekamp]

Wonderful, I just left a small comment on a stylistic issue but then we are ready to go.

[rainerhahnekamp]

Do you think it is possible to update the example with a utility function which encapsulates the access to environment?

I don't think it is a good idea if we promote spreading access to the environment in the codebase.

So something like

function withTreeShakableDevTools(featureName: string) {
  return environment.storeWithDevTolls(featureName);
}

[marcindz88]

I added an exported const which lets us not to import environment file directly, and allows for keeping the function signature internal to the library (may be useful if there were changes)

[rainerhahnekamp]

Thanks again @marcindz88