Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: ADR to make redux dependency optional #275

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions docs/decisions/0006-make-redux-deps-optional.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
Consumers of frontend-platform that don't use redux are still required to install redux dependencies
==========================================

Status
------

Review

Context
-------

Some consumers of frontend-platform, such as frontend-app-learner-portal-enterprise, no longer use redux or react-redux.
However, they are still required to install these dependencies due to the usage of react-redux in AppProvider, and the fact that
binodpant marked this conversation as resolved.
Show resolved Hide resolved
react-redux and redux are peerDependencies. To be precise, the OptionalReduxProvider used by AppProvider is what uses the
react-redux Provider in case a store prop is passed to AppProvider.

Specifically, if an app like frontend-app-learner-portal-enterprise were to uninstall the redux and
react-redux dependencies, it fails with an error during module loading like this::

ERROR in ./node_modules/@edx/frontend-platform/react/OptionalReduxProvider.js 3:0-39
Module not found: Error: Can't resolve 'react-redux' in 'frontend-app-learner-portal-enterprise/node_modules/@edx/frontend-platform/react'

Therefore, redux and react-redux are only optional from a code perspecitve, not from a dependency perspective right now.
This means there is extra overhead and confusion on seeing 'redux' and 'react-redux' in such projects, when they are not leveraging the `store` feature of AppProvider.

It will be great to be able to not have to include redux and react-redux as dependencies in such cases.

Decision
--------

A method to enable the removal of the dependencies `redux` and `react-redux` from mentioned client projects is to leverage optionalDependencies.

According to `npm documentation <https://docs.npmjs.com/cli/v8/configuring-npm/package-json#optionaldependencies>`_:

> optionalDependencies are dependencies that do not necessarily need to be installed.
If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install,
then you may put it in the optionalDependencies object.

Therefore if we were to move the redux and react-redux into optionalDependencies instead, consumers will no longer fail build
when these deps are missing.

In addition, since the `OptionalReduxProvider` makes use of the import of react-redux, it may need to also make a safe import such that
a missing package react-redux does not cause a failure, as noted in the above link. Instead it can simply detect the absence of the package
and return, something like this::

if (store === null || !Provider) {
return (
<>{children}</>
);
}
binodpant marked this conversation as resolved.
Show resolved Hide resolved


Consequences
--------

For consumers that use frontend-platform but do make use of the redux store feature of AppProvider, there should be no impact.

However, for consumers that do not use that feature, they should no longer need to depend on redux or react-redux.
binodpant marked this conversation as resolved.
Show resolved Hide resolved

Also, this will help bundle sizes to be smaller for consumers where this exclusion applies.