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

Enhancement: Design better module dependency handling #852

Merged

Conversation

maryamkhidir
Copy link
Contributor

@maryamkhidir maryamkhidir commented Nov 2, 2023

Feature or Bugfix

  • Enhancement
  • Refactoring

Detail

  • A solution for Remove hardcoded references in frontend/src/utils/helpers/moduleUtils.js and design better module dependency handling #847.
  • Removed hardcoded references in frontend/src/utils/helpers/moduleUtils.js
  • To ensure the independence of each module, prevent overwrites and avoidable conflicts in a single file moduleUtils.js. A new approach was designed to decouple the isModuleEnabled function in the moduleUtils file.
  • All modules are imported from the global config.json file, so when new modules are added, it is automatically loaded
  • Each module's dependency is handled in the module's index.js file using resolve_dependency() as part of the module's exports.

@dlpzx edits:

  • Adapted features from V2.1: added Notifications module
  • Adapted features from V2.1: add isFeatureEnable function (no changes)
  • Adapted features from V2.1: renamed isFeatureEnable function to isAnyEnvironmentModuleEnabled
  • Defined index.js for each module where we export an Object containing the module definition
  • Modified the ModuleNames definition to read from the module index definition
  • Designed a way to remove hardcoded references in isAnyEnvironmentModuleEnabled by using module index definition

Relates

#847

Security

N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@maryamkhidir
Copy link
Contributor Author

maryamkhidir commented Nov 2, 2023

@dlpzx @noah-paige wdyt?
PS: I can't add reviewers.

@anmolsgandhi anmolsgandhi added this to the v2.2.0 milestone Nov 7, 2023
@noah-paige
Copy link
Contributor

We will have to add similar for NOTIFICATIONS - however notifications does not have a structure in frontend similar to catalog, worksheets, glossaries, or shares

Will likely have to do some refactoring of the frontend for this if we want to keep this structure moving forward to identifying frontend dependencies and conditional rendering

@dlpzx
Copy link
Contributor

dlpzx commented Nov 9, 2023

As @noah-paige pointed out, we need to account for changes introduced in V2.1 release. @maryamkhidir can you merge the latest changes in main to your branch?

@dlpzx
Copy link
Contributor

dlpzx commented Nov 14, 2023

@maryamkhidir I have implemented and tested all the changes that we spoke about :) I will ask a second reviewer to review the PR so that we get another perspective onto the design

@dlpzx dlpzx requested a review from nikpodsh November 14, 2023 15:49
@@ -494,7 +497,7 @@ const EnvironmentEditForm = (props) => {
</FormGroup>
</Box>
)}
{isModuleEnabled(ModuleNames.PIPELINES) && (
{isModuleEnabled(ModuleNames.DATAPIPELINES) && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our next step (in other PR) can be extracting all the code where we check isModuleEnabled(module) into separate files. It should improve the code maintenance and reduce number of merge conflicts. I can imagine if a customer does some customization on pipelines and has to rebase and merge conflicts with a new dataset changes from main since everything in the same file :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understood you, you mean as components created in the module folder that are imported into the environment view? My idea for this view was to define this particular block in a more abstract way. Instead of hardcoding the different environment modules, I would prefer to have some sort of logic that creates the blocks based on the information in the index files + config activation status of the modules. But looking at it now i think we can combine both approaches:

  • iterate over the environment modules and call isModuleEnabled
  • import the correspondent environment-module-component inside the condition of isModuleEnabled

Copy link
Contributor

@dlpzx dlpzx Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can create a new issue for this particular problem + review if we can move some of the current code from frontend/services into frontend/modules/MODULENAME/services given that we define the dependencies well. Based on feedback from customers, I will also propose to create another directory frontend/core/ where we place modules such as Environment. Wdyt? Does this make sense or is it against frontend design rules?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoding the different environment modules, I would prefer to have some sort of logic that creates the blocks based on the information in the index files

This is a good idea.

I will also propose to create another directory frontend/core/ where we place modules such as Environment. Wdyt? Does this make sense or is it against frontend design rules?

I believe it's a good idea to consider which worth it's own ticket. I'd like that we gather prons and cons for it ( + we could try to start writing ADR as we had discussed before)

@dlpzx dlpzx merged commit f5c79e8 into data-dot-all:main Nov 16, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants