L is not defined in leaflet-iiif

projectblacklight / spotlight

Spotlight enables librarians, curators, and others who are responsible for digital collections to create attractive, feature-rich websites that highlight these collections.

Other

161 stars 65 forks source link

L is not defined in leaflet-iiif #2961

Open jcoyne opened 1 year ago

jcoyne commented 1 year ago

When loaded as a module. It may depend on L as a constant (via Leaflet)

https://github.com/projectblacklight/spotlight/blob/main/vendor/assets/javascripts/leaflet-iiif.js#L7

The original is in @mejackreed's repo: https://github.com/mejackreed/Leaflet-IIIF

mejackreed commented 1 year ago

Has an absolute hard dependency on Leaflet

jcoyne commented 1 year ago

Hi @mejackreed 👋 I'm wondering it it can be patched to import L from "leaflet-src.esm.js" rather than expectingL to be defined globally?

mejackreed commented 1 year ago

For the fork here in Spotlight, sure? Leaflet plugins usually follow a pattern to extend L from an expected global existence. Maybe there are other patterns out there to handle this?

Also, I have no understanding anymore about how Rails expects this to work 😉

// leaflet-iiif importer

import L from "leaflet-src.esm.js";
import leaflet-iiif

jcoyne commented 1 year ago

@mejackreed Using ESM modules seems to be the path forward for Leaflet:

import leaflet/dist/leaflet-src.esm.js explicitly instead to take advantage [of modules]; ESM by default will come in v2

https://github.com/Leaflet/Leaflet/releases/tag/v1.9.2

jcoyne commented 1 year ago

Here is a PR that would resolve this: https://github.com/mejackreed/Leaflet-IIIF/pull/93

taylor-steve commented 2 months ago

Noting that:

Justin's work here makes Leaflet-IIIF work with import maps. I've been using it in my exploratory BL8 builds in Spotlight and it works great for that. If we can't get it into the gem, we might need to make a decision (e.g., fork or apply the patch locally in Spotlight).
@cbeer pointed out that Leaflet-IIIF is likely only used for one or two purposes in Spotlight (e.g., image cropping). Now that we're willing to introduce breaking changes, another valid approach to this ticket could be to replicate/replace that functionality using a different approach and removing the dependency on Leaflet-IIIF.

taylor-steve commented 2 months ago

Confirming it appears this is only used in Spotlight for the cropper in admin/crop.js.

It seems we could replicate this cropping tool using OSD or OpenLayers, but it'd be work. It might be worth waiting until we know more about https://github.com/sul-dlss/exhibits/issues/357 on the off chance we need to support something beyond a fixed aspect ratio rectangle.

If the existing tool is sufficient, I'd suggest we wait until we're slightly further in the overall JS work.

taylor-steve commented 2 months ago

@dnoneill suggested https://github.com/annotorious/annotorious which looks promising should we want to switch to OSD.

taylor-steve commented 2 months ago

I think we may want to explore the replacement option for the cropper further. If it's on the order of ~1-2 days of work, it may be faster than trying to resolve all the quirks it is having in the Propshaft build.