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

Add index.js files to src directories #3991

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Add index.js files to src directories #3991

merged 1 commit into from
Dec 13, 2024

Conversation

cbeer
Copy link
Collaborator

@cbeer cbeer commented Nov 19, 2024

Pushing most of the classes up to the top level should make downstream imports more robust (vs the current file-based mirador/dist/es/*imports).

@cbeer cbeer marked this pull request as ready for review November 19, 2024 18:10
@lutzhelm
Copy link
Contributor

lutzhelm commented Dec 6, 2024

One thing I noticed is that there is just a single default export in the index.js. As a result, this export always need to be imported entirely. Is that intentional?

E.g. instead of importing just the necessary items with a named import like this:

import { ScrollIndicatedDialogContent } from 'mirador';

... the items of interest now always have to be taken from the default import.

import Mirador from 'mirador';
const { ScrollIndicatedDialogContent } = Mirador;

It works fine, but I'm not sure if this can become cumbersome. (I might also overlook an easier way to handle this.)

@cbeer
Copy link
Collaborator Author

cbeer commented Dec 6, 2024

Thanks, that wasn't my intent. I guess I should have been more suspicious of the existing code.

@lutzhelm
Copy link
Contributor

lutzhelm commented Dec 9, 2024

Redux state related things like actions, selectors, reducers still need to be imported en bloc, but I am not sure if it would be advisable expose every action, selector etc. individually via index.js.

I also stumbled upon some imports in the plugins that still need to be imported with the entire dist/es/src path, e.g. mirador/dist/es/src/config/settings or mirador/dist/es/src/state/reducers/rootReducer. They are only used to set up Context Providers for the tests. It might be possible to run those tests without those imports. And if they are actually needed, the full path imports are still available.

@cbeer
Copy link
Collaborator Author

cbeer commented Dec 9, 2024

I've tweaked the state/index.js re-exports so maybe it's better now. There's some subtlety (maybe with default exports?) I'm not fully grasping.

... and I've added those two as exports. 🤷‍♂️

@lutzhelm
Copy link
Contributor

I gave this branch a try with @gerdesque's branch of the mirador-textoverlay plugin. I noticed two things:

  • The plugin currently uses mirador/dist/es/src/state/actions/action-types which is not exposed through index.js yet.
  • There is a weird error with the MiradorCanvas export. Everything works fine if it is imported from mirador/dist/es/src/lib/MiradorCanvas, but as soon as I tried a named import from mirador/dist/es/src/lib or mirador, I get an error that it is not a constructor. This happens with webpack (mirador__WEBPACK_IMPORTED_MODULE_10__ is not a constructor) as well as with parcel (_lib.MiradorCanvas is not a constructor). However, if I haven't entirely trashed my project folder, it seems like the respective MiradorCanvas import has a default property that is actually a constructor.

(Sorry that for all these individual comments instead of a single review.)

@cbeer cbeer force-pushed the index-js branch 2 times, most recently from cc00a42 to a20cf4c Compare December 11, 2024 16:15
@cbeer
Copy link
Collaborator Author

cbeer commented Dec 11, 2024

Ok, try this? Our mess of default and non-default exports make this pretty confusing.

@cbeer cbeer force-pushed the index-js branch 3 times, most recently from 64c15e9 to e1df7e7 Compare December 11, 2024 17:16
@lutzhelm
Copy link
Contributor

I have to update my findings for today:

  • mirador-dl-plugin: works!
  • mirador-textoverlay: state seems to work, but the controls don't show up
  • mirador-image-tools: needs withRef and withSize references removed, but controls still don't show up

I was indeed not aware of the OSD changes, e.g. #4014; it could be that the plugins just have to be adapted to those changes.

Copy link
Contributor

@lutzhelm lutzhelm left a comment

Choose a reason for hiding this comment

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

Everything looks fine now; got all three plugins to work that I tested before.

@cbeer cbeer merged commit 3abaca3 into master Dec 13, 2024
8 checks passed
@cbeer cbeer deleted the index-js branch December 13, 2024 15:33
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.

2 participants