From 737e3e761251a8b96c5b02915e307876c3b85aa3 Mon Sep 17 00:00:00 2001 From: binodpant Date: Fri, 14 Jan 2022 13:00:12 -0500 Subject: [PATCH 1/4] docs: ADR to make redux dependency optional --- .../0006-make-redux-deps-optional.rst | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 docs/decisions/0006-make-redux-deps-optional.rst diff --git a/docs/decisions/0006-make-redux-deps-optional.rst b/docs/decisions/0006-make-redux-deps-optional.rst new file mode 100644 index 000000000..0d428e1f7 --- /dev/null +++ b/docs/decisions/0006-make-redux-deps-optional.rst @@ -0,0 +1,51 @@ +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 +react-redux and redux are peerDependencies. + +This means there is extra overhead and confusion 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: + +> 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. Instead it can simply detect the absence of the package +and return, something like this + +``` +if (store === null || !Provider) { + return ( + <>{children} + ); + } +``` + +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. From 12b38d4425f9979deaf0d2647644215c6f3ba6eb Mon Sep 17 00:00:00 2001 From: binodpant Date: Fri, 14 Jan 2022 13:04:19 -0500 Subject: [PATCH 2/4] docs: format messup --- docs/decisions/0006-make-redux-deps-optional.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/decisions/0006-make-redux-deps-optional.rst b/docs/decisions/0006-make-redux-deps-optional.rst index 0d428e1f7..76fdd9e8e 100644 --- a/docs/decisions/0006-make-redux-deps-optional.rst +++ b/docs/decisions/0006-make-redux-deps-optional.rst @@ -35,13 +35,13 @@ In addition, since the `OptionalReduxProvider` makes use of the import of react- a missing package react-redux does not cause a failure. Instead it can simply detect the absence of the package and return, something like this -``` -if (store === null || !Provider) { - return ( - <>{children} - ); - } -``` + + if (store === null || !Provider) { + return ( + <>{children} + ); + } + Consequences -------- From 2af6e11a67c16288d3ef49cd9764a7788a9a1675 Mon Sep 17 00:00:00 2001 From: binodpant Date: Fri, 14 Jan 2022 13:51:13 -0500 Subject: [PATCH 3/4] docs: updates to doc --- docs/decisions/0006-make-redux-deps-optional.rst | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/decisions/0006-make-redux-deps-optional.rst b/docs/decisions/0006-make-redux-deps-optional.rst index 76fdd9e8e..f7413f805 100644 --- a/docs/decisions/0006-make-redux-deps-optional.rst +++ b/docs/decisions/0006-make-redux-deps-optional.rst @@ -11,9 +11,12 @@ 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 -react-redux and redux are peerDependencies. +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. -This means there is extra overhead and confusion in such projects, when they are not leveraging the `store` feature of AppProvider. +However, it's only optional from a code perspecitve, it's not optional 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. @@ -22,7 +25,7 @@ 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: +According to `npm documentation `_: > 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, @@ -32,9 +35,8 @@ Therefore if we were to move the redux and react-redux into optionalDependencies 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. Instead it can simply detect the absence of the package -and return, something like this - +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 ( @@ -49,3 +51,5 @@ 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. + +Also, this will help bundle sizes to be smaller for consumers where this exclusion applies. From d091cc02bc7747d97d58e76cfa4fe2b3a069e485 Mon Sep 17 00:00:00 2001 From: binodpant Date: Fri, 14 Jan 2022 13:54:45 -0500 Subject: [PATCH 4/4] docs: clarify how things fail --- docs/decisions/0006-make-redux-deps-optional.rst | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/decisions/0006-make-redux-deps-optional.rst b/docs/decisions/0006-make-redux-deps-optional.rst index f7413f805..16117896d 100644 --- a/docs/decisions/0006-make-redux-deps-optional.rst +++ b/docs/decisions/0006-make-redux-deps-optional.rst @@ -14,8 +14,13 @@ However, they are still required to install these dependencies due to the usage 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. -However, it's only optional from a code perspecitve, it's not optional from a dependency perspective right now. +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.