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

Remove material-ui dependency #3171

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

pastr
Copy link
Contributor

@pastr pastr commented Jul 7, 2024

References

None

Description

Remove the material-ui dependency, it's not used and it's a React library.

Instructions for Reviewers

The application works the same without these dependencies. These dependencies are not used in the codebase.

List of changes in this PR:

  • Removed @material-ui/core and @material-ui/icons dependencies

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@alanorth
Copy link
Contributor

alanorth commented Jul 8, 2024

Thanks @pastr. I verified there are no imports of material-ui anywhere in our Angular code and building the application still works with them removed.

P.S. I think you also need to commit the changes to yarn.lock.

@alanorth alanorth added dependencies Pull requests that update a dependency file port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Jul 8, 2024
@pastr
Copy link
Contributor Author

pastr commented Jul 8, 2024

@alanorth Oh you are right, I forgot about it.
It's done 👍

@tdonohue tdonohue self-requested a review July 9, 2024 14:17
Copy link

github-actions bot commented Sep 6, 2024

Hi @pastr,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

@tdonohue
Copy link
Member

tdonohue commented Sep 6, 2024

@pastr : The conflicts in this PR are because we recently switched from yarn to just npm (so the yarn.lock is replaced with package-lock.json). See #3173

If you could find time to update this, I still think this is a necessary change. If not, I might be able to find time to rework this in the future. Apologies it's taken so long to review, but I'd like to finally get this merged.

@pastr
Copy link
Contributor Author

pastr commented Sep 6, 2024

hello @tdonohue
Happy to learn about the switch to npm!
I found 2 others react dependencies (Prop-types and react-copy-to-clipboard) that were not used and also removed them.
I noticed that my editor automatically fixed some indentation in package.json but it seems more consistent that way (2 spaces indentation everywhere).

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @pastr ! I was able to give this another look/test today. Looks good. I also built the system with this PR installed and can verify that there doesn't seem to be any change in behavior.

@tdonohue tdonohue added this to the 9.0 milestone Sep 10, 2024
@tdonohue tdonohue removed port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Sep 10, 2024
@tdonohue tdonohue merged commit 16943fe into DSpace:main Sep 10, 2024
13 checks passed
@tdonohue
Copy link
Member

tdonohue commented Sep 10, 2024

After merging this, I've realized that these material-ui dependencies might be required for sites using Mirador IIIF viewer (https://wiki.lyrasis.org/display/DSDOC7x/IIIF+Configuration).

When I looked at backporting this to 8.x / 7.x, yarn install returned these new warnings:

warning " > mirador-dl-plugin@0.13.0" has unmet peer dependency "@material-ui/core@^4.11.0".
warning " > mirador-dl-plugin@0.13.0" has unmet peer dependency "@material-ui/icons@^4.9.1".
warning " > mirador-dl-plugin@0.13.0" has unmet peer dependency "prop-types@^15.7.2".
warning " > mirador-share-plugin@0.11.0" has unmet peer dependency "@material-ui/core@^4.7.2".
warning " > mirador-share-plugin@0.11.0" has unmet peer dependency "@material-ui/icons@^4.5.1".
warning " > mirador-share-plugin@0.11.0" has unmet peer dependency "prop-types@^15.7.2".
warning " > mirador-share-plugin@0.11.0" has unmet peer dependency "react-copy-to-clipboard@^5.0.1".

Digging into those mirador-*-plugins shows they are enabled by default in the Mirador Viewer Settings here:
https://github.com/DSpace/dspace-angular/blob/main/src/mirador-viewer/config.default.js#L173-L176

That implies to me that we might need to undo this PR, unfortunately.

@mspalti: Do you happen to know if we need to include these for Mirador to function properly? Maybe we need to move these dependencies over to "optional" (as not all sites will be using Mirador)?

@mspalti
Copy link
Member

mspalti commented Sep 10, 2024

There will be a few react dependencies because Mirador, obviously, uses react.

In the case the download and share plugins, these are not required. If we remove them from the default Mirador configuration the viewer will work. We'd need to test that, obviously. The share plugin is sort of a core feature because it's a handy tool for embedding DSpace Manifests on other sites (i.e. the interoperability part of the IIIF framework). But I wouldn't call it essential for an installation that's only concerned with embedding an image viewer in a DSpace Item. For anyone wanting more than this I think they'd want these two plugins.

btw, It looks like there's a newer version of mirador-share-plugin. We might test that. And perhaps work a bit on fixing the material dependency warnings. In my experience, the missing deps haven't caused problems with functionality.

@tdonohue
Copy link
Member

@mspalti : Thanks for the verification that removing these dependencies shouldn't be a big deal. I agree it'd be nice to upgrade any Mirador plugins that we can.

For now, I'm going to refrain from backporting this change as it seems like it may need more testing that nothing "breaks" with IIIF when removing these material-ui dependencies. Once that is verified, we could consider backporting this to 8.x and 7.x (but either would require a manual backport).

@tdonohue
Copy link
Member

tdonohue commented Oct 25, 2024

NOTE: I finally got around to fully testing Mirador / IIIF Integration after the removal of these dependencies. I'm glad to report that Mirador works perfectly without material-ui or the other dependencies removed in this PR. I was also able to remove react and react-dom (both of which are only included for Mirador) in #3553 , and Mirador still works well.

That said, I wanted to also note that this PR will not be backported to 8.x or 7.x (both ofwhich use Yarn instead of NPM). The reason is that yarn install will throw a large number of "unmet peer dependency" warnings when all these Mirador-related dependencies are fully removed. The better solution for 8.x and 7.x seems to be to move the Mirador-related dependencies all to "dev" dependencies (as they are not needed in production or at runtime). I've already done that in #3573 (for 8.x), and verified that works best for yarn install and Mirador continues to work well. This same concept will be ported to 7.x as well.

@tdonohue tdonohue added the integration: IIIF Related to International Image Interoperability Framework (IIIF) support label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file integration: IIIF Related to International Image Interoperability Framework (IIIF) support
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants