ProjectMirador / mirador

An open-source, web-based 'multi-up' viewer that supports zoom-pan-rotate functionality, ability to display/compare simple images, and images with annotations.
https://projectmirador.org
Apache License 2.0
548 stars 258 forks source link

Discourage importing specific module formats directly #3480

Open charbugs opened 3 years ago

charbugs commented 3 years ago

I see a lot of implementations and plugins that import Mirador or parts of it by targeting the es or cjs folders directly, e.g. import Mirador from "mirador/dist/es/index. I think this is bad practice as it may break the tooling (bundling, testing and so on) in many ways. This is not to blame the people for that because we gave a lot of examples in the past that suggested doing this.

Usually you compile your library code into different module formats and than give pointers in the package.json to the locations of that formats via the fields "main", "module", "browser", "exports" and some more. Than the library consumers import that code without explicitly point to a specific module format e.g. import Mirador from 'mirador'. Now if you run tools agains that code the tools can decide what kind of modules to import. For example, Webpack prefers ES modules and expects the "browser" or "module" field to point to that modules. If this fields are not present it falls back to the "main" field that usually points to CJS modules. Jest in turn can only work with CJS modules and always targets the "main" field for that reason. If you import code from a specific module format you force the tools to use this format even if it is not appropriate.

For people who simply want to run the viewer there should only be two encouraged ways: Either embedding the browser bundle via script tag from a CDN:

<script src="https://unpkg.com/mirador@3.2.0/dist/mirador.min.js"</script>
<div id="mirador-container"></div>
<script>
  Mirador.viewer({ id: 'mirador-container', /* more config here */ })
</script>

Or importing the constructor from an NPM package and than bundle the code:

import Mirador from 'mirador' /* NO DIRECT IMPORT OF ES OR CJS MODULES */
const mirador = Mirador.viewer({ id: 'mirador-container', /* more config here */ })

Now if you want to create a plugin for Mirador depending on the complexity of that plugin you have to import more low level functions of Mirador. For example, here is a list of import statements from different plugins:

import { miradorSlice } from 'mirador/dist/es/src/state/selectors/utils';
import ActionTypes from 'mirador/dist/es/src/state/actions/action-types';
import { MiradorMenuButton } from 'mirador/dist/es/src/components/MiradorMenuButton';
import WindowMaxIcon from 'mirador/dist/es/src/components/icons/WindowMaxIcon'
import ns from 'mirador/dist/es/src/config/css-ns'
import { OSDReferences } from 'mirador/dist/es/src/plugins/OSDReferences';

Actually, the redux state functions don't need to be imported that way because Mirador exports them at the top level. But all other functions and components are not exported so there is no other way than use the ES (or CJS) modules directly. If you want to use this plugins in your app you may again run in the same tooling problems describe above even if you follow the good practice of not importing ES modules directly in your own code.

Of course, we could export more functions and components to the top level and that would solve the problem. But I hesitate because that would mean to expose implementation details to the surface. So I like to know, are there any ideas how we could go about this?

Finally the whole problem is based on the fact that the plugin system has no sensible restrictions and every plugin I've seen so far is using implementation details that will change and result in plugins not working anymore without us having a chance to communicate breaking changes at new releases. So if there are ideas on that basic problem I would be happy to here them.

charbugs commented 3 years ago

What about this:

This wouldn't break any existing plugin and we could gradually decide what functions to include into or remove from this api.

charbugs commented 3 years ago

I searched on Github for Mirador 3 plugins and assembled a list of all Mirador import paths to see what low level functions they use. The list boils down to this paths:

components/MiradorMenuButton components/WindowTopBarPluginMenu components/icons/CanvasIndexIcon components/icons/WindowMaxIcon' components/icons/WindowMinIcon config/css-ns containers/CompanionWindow containers/ScrollIndicatedDialogContent extend/withPlugins' lib/MiradorCanvas plugins/OSDReferences state/actions state/actions/action-types state/selectors

Here is the exented list:

https://github.com/dbmdz/mirador-textoverlay

import { updateWindow } from 'mirador/dist/es/src/state/actions'; import { getWindowConfig, getContainerId } from 'mirador/dist/es/src/state/selectors'; import { MiradorMenuButton } from 'mirador/dist/es/src/components/MiradorMenuButton'; import ActionTypes from 'mirador/dist/es/src/state/actions/action-types'; import { receiveAnnotation, updateConfig } from 'mirador/dist/es/src/state/actions'; import { getCanvases, getWindowConfig, getVisibleCanvases, selectInfoResponse, } from 'mirador/dist/es/src/state/selectors'; import MiradorCanvas from 'mirador/dist/es/src/lib/MiradorCanvas'; import { getWindowConfig, getVisibleCanvases, getTheme } from 'mirador/dist/es/src/state/selectors'; import { miradorSlice } from 'mirador/dist/es/src/state/selectors/utils';

https://github.com/projectmirador/mirador-annotations

import { MiradorMenuButton } from 'mirador/dist/es/src/components/MiradorMenuButton'; import mirador from 'mirador/dist/es/src/index'; import CompanionWindow from 'mirador/dist/es/src/containers/CompanionWindow'; import { OSDReferences } from 'mirador/dist/es/src/plugins/OSDReferences'; import as actions from 'mirador/dist/es/src/state/actions'; import { getCompanionWindow } from 'mirador/dist/es/src/state/selectors/companionWindows'; import { getVisibleCanvases } from 'mirador/dist/es/src/state/selectors/canvases'; import { getVisibleCanvases } from 'mirador/dist/es/src/state/selectors/canvases'; import as actions from 'mirador/dist/es/src/state/actions'; import { getWindowViewType } from 'mirador/dist/es/src/state/selectors'; import as actions from 'mirador/dist/es/src/state/actions'; import { getVisibleCanvases } from 'mirador/dist/es/src/state/selectors/canvases'; import as actions from 'mirador/dist/es/src/state/actions'; import { getWindowViewType } from 'mirador/dist/es/src/state/selectors'; import { MiradorMenuButton } from 'mirador/dist/es/src/components/MiradorMenuButton'; import { getVisibleCanvases } from 'mirador/dist/es/src/state/selectors/canvases';

https://github.com/ProjectMirador/mirador-annotot-endpoint-plugin

mirador.selectors.getVisibleCanvases(state, { windowId: targetProps.windowId }), mirador.actions.fetchAnnotation

https://github.com/HarvardX/catchpy-mirador-plugin

mirador.selectors.getSelectedCanvas(state, windowId), mirador.actions.receiveAnnotation

https://github.com/ProjectMirador/mirador-image-tools

import { MiradorMenuButton } from 'mirador/dist/es/src/components/MiradorMenuButton'; import * as actions from 'mirador/dist/es/src/state/actions'; import { getWindowConfig, getViewer, getContainerId } from 'mirador/dist/es/src/state/selectors';

https://github.com/ProjectMirador/mirador-share-plugin

import Mirador from 'mirador/dist/es/src/index'; import { getManifestoInstance } from 'mirador/dist/es/src/state/selectors/manifests'; import { getContainerId } from 'mirador/dist/es/src/state/selectors/config'; import ScrollIndicatedDialogContent from 'mirador/dist/es/src/containers/ScrollIndicatedDialogContent'; import { getManifestoInstance } from 'mirador/dist/es/src/state/selectors/manifests';

https://github.com/ProjectMirador/mirador-dl-plugin

import { OSDReferences } from 'mirador/dist/es/src/plugins/OSDReferences'; import Mirador from 'mirador/dist/es/src/index'; import { getCanvasLabel, getVisibleCanvases, selectInfoResponse } from 'mirador/dist/es/src/state/selectors/canvases'; import { getWindowViewType } from 'mirador/dist/es/src/state/selectors/windows'; import { getManifestoInstance } from 'mirador/dist/es/src/state/selectors/manifests'; import { getContainerId } from 'mirador/dist/es/src/state/selectors/config'; import ScrollIndicatedDialogContent from 'mirador/dist/es/src/containers/ScrollIndicatedDialogContent';

Handschriftenportal Manuscript Description Plugin

import MiradorMenuButton from 'mirador/dist/es/src/containers/MiradorMenuButton' import WindowMaxIcon from 'mirador/dist/es/src/components/icons/WindowMaxIcon' import WindowMinIcon from 'mirador/dist/es/src/components/icons/WindowMinIcon' import * as m3actions from 'mirador/dist/es/src/state/actions' import { getWindow } from 'mirador/dist/es/src/state/selectors' import ns from 'mirador/dist/es/src/config/css-ns' import ns from 'mirador/dist/es/src/config/css-ns' import { getWindow } from 'mirador/dist/es/src/state/selectors' import CanvasIndexIcon from 'mirador/dist/es/src/components/icons/CanvasIndexIcon' import MiradorMenuButton from 'mirador/dist/es/src/containers/MiradorMenuButton' import { getCanvasLabel, getVisibleCanvases, selectInfoResponses } from 'mirador/dist/es/src/state/selectors/canvases' import { getConfig } from 'mirador/dist/es/src/state/selectors/config' import MiradorCanvas from 'mirador/dist/es/src/lib/MiradorCanvas'

https://github.com/durham-university/mirador-content-ScrollIndicatedDialogContent

import ActionTypes from 'mirador/dist/es/src/state/actions/action-types'; import { getWindow } from 'mirador/dist/es/src/state/selectors/getters'; import { receiveAnnotation, updateViewport } from 'mirador/dist/es/src/state/actions'; import { getCompanionWindowsForContent } from 'mirador/dist/es/src/state/selectors'; import { updateCompanionWindow, addWindow } from 'mirador/dist/es/src/state/actions';

https://github.com/jeffreycwitt/mirador3-ldn-plugin

import { receiveManifest } from 'mirador/dist/es/src/state/actions/manifest'; import { removeInfoResponse } from 'mirador/dist/es/src/state/actions/infoResponse';

https://github.com/art-institute-of-chicago/aic-mirador-ui

import * as actions from 'mirador/dist/es/src/state/actions'; import { getSequenceViewingDirection, getNextCanvasGrouping, getPreviousCanvasGrouping, } from 'mirador/dist/es/src/state/selectors'; import { getViewer } from 'mirador/dist/es/src/state/selectors'; import { withPlugins } from 'mirador/dist/es/src/extend/withPlugins'; import { WindowTopBarPluginMenu } from 'mirador/dist/es/src/components/WindowTopBarPluginMenu'; import { getContainerId } from 'mirador/dist/es/src/state/selectors';

https://github.com/ubleipzig/mirador-ruler-plugin

import { getContainerId } from 'mirador/dist/es/src/state/selectors' import { getSelectedCanvases } from 'mirador/dist/es/src/state/selectors';

jbaiter commented 3 years ago

Finally the whole problem is based on the fact that the plugin system has no sensible restrictions and every plugin I've seen so far is using implementation details that will change and result in plugins not working anymore without us having a chance to communicate breaking changes at new releases. So if there are ideas on that basic problem I would be happy to here them.

Youre :100: on this, this is currently a blessing and a curse both at once. A blessing, since it gives a lot of freedom to plugin authors to override and extend pretty much every part of Mirador at their will. And a curse, since as you correctly state, this is going to cause a ton of pain down the road when internal APIs change and plugins break left and right.

Your proposal seems reasonable, it could serve as a starting point for working out something like a "officially endorsed and stable plugin API". There will certainly be cases where new plugins need new APIs, but it should be possible to have a very streamlined process to push more APIs into the proposed lowevel namespace.

Maybe we need to take a close look not only at which APIs are currently used by plugins, but also what kinds of plugins we want to allow and what APIs are required for these use cases. It's certainly going to be challenging, especially with wrap-type plugins that clone existing components and only change them in one or two places. The current pattern for these seems to be to copy&paste the existing component, change the imports to point to mirador/dist/es/... and then apply the modifications.

I definitely think that import ... from 'mirador/dist/es/...' will not go away for the foreseeable future, and it's probably not going to be a huge issue in a lot of contexts (internal plugins, customized Mirador installations, etc), but it would certainly be great to have something a bit more "officially endorsed" to guide new plugin authors to more stable paths until they need more power.

charbugs commented 3 years ago

@jbaiter Thank you for your reply. I'm currently in holiday and continue to work on that when I'm back in two weeks.

charbugs commented 3 years ago

@jbaiter

I definitely think that import ... from 'mirador/dist/es/...' will not go away for the foreseeable future, and it's probably not going to be a huge issue in a lot of contexts (internal plugins, customized Mirador installations, etc)

As I pointed out, it may be a problem in many ways. Take for example a plugin that imports from ES directly and another plugin (or the host app itself) that imports from CJS or UMD and than compare the bundle sizes after compiling with webpack:

Mirador without plugins:

import Mirador from 'mirador'
Mirador.viewer({ id: 'mirador-container' })
$ du -hc dist/*.js | tail -n 1
1.8M total

Mirador with a plugin that imports from ES:

import Mirador from 'mirador'
import textOverlayPlugin from 'mirador-textoverlay'
Mirador.viewer({ id: 'mirador-container' }, [...textOverlayPlugin])
$ du -hc dist/*.js | tail -n 1
2.0M total

Import Mirador from CJS for some reason (there may also be another plugin that imports from CJS)

import Mirador from 'mirador/dist/src/cjs'
import textOverlayPlugin from 'mirador-textoverlay'
Mirador.viewer({ id: 'mirador-container' }, [...textOverlayPlugin])
$ du -hc dist/*.js | tail -n 1
2.3M total

As you can see from the last example there is a code duplication of 300K. And the larger bundle size is not even the biggest problem. What if the duplicated code has side effects while importing?