ES6 module export of GeoTIFFTileSource

[abiswas97]

Hi @pearcetm,

Apologies for the pull request - opening this to discuss this implementation, and I don't intend to merge unless it's actually useful.

I've been meaning to incorporate GeoTIFFTileSource into my app for a while, but my main hurdle had been converting the current IIFE into an ES6 module. My use case is a React based tissue slide annotator, and I wanted to support local and remote SVS / TIFF / OME-TIFF files without the need for conversion to DZI.

In this PR, I've taken a crack at converting the existing IIFE to a class wrapped in an exported function. I've wrapped the class in a function so that the codebase stays agnostic of OSD versions, and doesn't need to maintain an internal version of OSD. The internals of the plugin remain unchanged, but have been re-organised to fit the class based syntax.

The README has an example of how this could be initialized using ES6 imports. I've also bundled the package in vite, and it is currently available on npm.

I'd greatly appreciate any feedback on the approach, and to see if this could be potentially useful for anyone else.

Thanks again for this great plugin, it's been a massive resource for our use case :))

[pearcetm]

Thanks for opening this PR, @abiswas97! I'm glad to hear you are finding this to be a useful plugin :).

Until very recently I haven't really worked with React, so the plugin was really geared just toward vanilla js usage. However, I've finally started working with React on a number of projects over the past couple of months, and I have seen how useful it is to have code packaged for that use case too.

I've glanced at your changes so far, but I haven't had a chance to thoroughly review yet (plus, I'm still kind of a novice at the whole React game). I love the info you've added to the readme.md - it is great to have clear example code to start from! I definitely want to add support for packaging this so it can be incorporated into React projects and the like. Your help with this is greatly appreciated! Much like I'm new to React, I'm also just learning about Vite. It sounds pretty sweet, but I'm not experienced with it yet.

Big picture, I'd like the plugin to be usable via npm (packaged by Vite, webpack, whatever...), like you're working on, but I also want to make sure it is easy to use directly in the browser via vanilla js. As long as we can do both things, I'd be delighted to incorporate your changes.

More to come as I dig more into the the changes you've made in your fork. Looking forward to your thoughts and the upcoming discussion.

[abiswas97]

Allowing imports from both a script tag and npm does sound ideal. This was my first time packaging something, so I'm not too sure on the best way forward, but I'm planning to take a look at the build scripts that OpenSeadragon and Annotorious use. Annotorious uses vite, so that might shed some light on what to do.

My assumption is that the codebase could either -

• Maintain a separate build that could be included as a script tag
• Configure the build tool to output something as close to the current IIFE as possible on build

I'll take a look at vite's build configs and see what's possible

[pearcetm]

I'm new to vite myself, but I know in webpack you can target "Universal module definition" (umd):

type: 'umd' This exposes your library under all the module definitions, allowing it to work with CommonJS, AMD, and as global variable. Take a look at the UMD Repository to learn more.

In this case, you need the library.name property to name your module:

module.exports = {
  //...
  output: {
    library: {
      name: 'MyLibrary',
      type: 'umd',
    },
  },
};

And finally the output is:

(function webpackUniversalModuleDefinition(root, factory) {
  if (typeof exports === 'object' && typeof module === 'object')
    module.exports = factory();
  else if (typeof define === 'function' && define.amd) define([], factory);
  else if (typeof exports === 'object') exports['MyLibrary'] = factory();
  else root['MyLibrary'] = factory();
})(global, function () {
  return _entry_return_;
});

[swamidass]

Thanks for this PR. Seems to work as advertised for me. Solved a major problem.

[pearcetm]

Great! Thanks for testing, @swamidass. @abiswas97 do you think this is ready to merge or are you still working on additional changes?

[abiswas97]

@pearcetm sorry for not clarifying - this PR should be ready to merge!

A pending feature could be automation - eg. creating the package and auto deploying to npm when a release is created. I could add that to this PR as well. All we'll need on your end is adding an npm token to github secrets.

I am working on other changes in my fork of this repo, but they're not cleaned up yet. I've been adding support for multi-channel QPTIFF images, and could open a separate PR for that if you think it would be useful.

[pearcetm]

@pearcetm sorry for not clarifying - this PR should be ready to merge!

Great! Looks like you replied while I was reviewing the changes.

A pending feature could be automation - eg. creating the package and auto deploying to npm when a release is created. I could add that to this PR as well. All we'll need on your end is adding an npm token to github secrets.

This sounds like a good idea to me. I'm not experienced with the process for this, so I'll follow your lead.

I am working on other changes in my fork of this repo, but they're not cleaned up yet. I've been adding support for multi-channel QPTIFF images, and could open a separate PR for that if you think it would be useful.

If you'd like to keep them in this PR (even if only partial support) I'm fine with that. If you think it would be better to pull those out into a separate PR for a future release, I'm also fine with that.

[abiswas97]

Awesome, I'll set up a github action for the automation then.

The qptiff changes should be safe since they don't alter any core functionality for non-qptiff files. The only apparent change would be the exposed channels array.

P.S. Sorry for the attribution errors and lack of thought on back compat, will add these back

[swamidass]

Looks like this is coming along. Let us know when it's available on npm with this support too.

[pearcetm]

Looks like this is coming along. Let us know when it's available on npm with this support too.

Yes, @swamidass, getting very close! @abiswas97 are you up for making the final changes needed to get this merged?

[abiswas97]

@pearcetm sorry for the delay, have been swamped the past few weeks - I can get to the last changes later today

[abiswas97]

Pending change on my end is to modify my currently deployed npm package to be @abiswas97/geotiff-tilesource, so this repo doesn't have any naming conflicts. @pearcetm can you check if I've missed anything? I'll be more available this weekend so I'll get to it as soon as I can

[abiswas97]

I can add OSD to peerDependencies - but adding OSD to dependencies will force the npm package to download a specific version of OSD, on top of what the user already has. I could instead do

  "peerDependencies": {
    "openseadragon": "^3.0.0 || ^4.0.0"
  },

Thoughts on this?

[abiswas97]

Ah I misunderstood the attribution requirement - forgot that I'd need to manually update Converters in it's new location. All done!

[pearcetm]

I can add OSD to peerDependencies - but adding OSD to dependencies will force the npm package to download a specific version of OSD, on top of what the user already has. I could instead do

  "peerDependencies": {
    "openseadragon": "^3.0.0 || ^4.0.0"
  },

Thoughts on this?

Adding as a peer dependency makes sense to me.

[abiswas97]

As a last change, I've added all dist output into git - this is more of a convenience for my current projects that use my fork. The change allows for installing directly from github by doing

npm install git+https://git@github.com/abiswas97/GeoTIFFTileSource.git --save

[abiswas97]

@pearcetm does the current code look ok to merge? Once the merge is complete I'll remove my current package from npm so it can be replaced by the github action output from here

[pearcetm]

@pearcetm does the current code look ok to merge?

Thank you for making these changes so we can merge! Just a couple more things - a question and a request.

My question: what is the advantage of having a separate function (enableGeoTIFFTileSource) that must be called explicitly, rather than just attaching the class to the OpenSeadragon object automatically, as is done now? I know we are already breaking total backwards compatibility, but I wonder if it wouldn't be more straightforward to just attach the GeoTIFFTileSource class automatically, to minimize the change in behavior.

My request: I'd prefer that we include either 1) a non-minified version of the built library that could be created alongside the minimized version, and/or 2) a source map. Having one or both of these available is really useful for debugging in the browser. Is this doable? (I'm not an expert in vite at all, so you'll have to help out!)

[abiswas97]

To be very fair I'm very early in my days as a dev, so I might not be truly accurate - but I can share my current understanding on this.

The current approach works better with explicitly modular JS, where scripts have explicit boundaries and have to be predictable - so they don't alter objects in the global namespace. While the module still extends OpenSeadragon, it can't make assumptions about OSD being in the client's global namespace and must have it passed down explicitly. It also can't access OSD using window.OpenSeadragon, since it can't assume that OSD is in the global namespace.

What I could do for backwards compat, is have an IIFE file that calls the current enableGeoTIFFTileSource on the window.OpenSeadragon object if present. But this would only work if OSD was also imported in a HTML script tag, and will not work if OSD is installed through NPM and imported in a module.

For the non minified and the source map, I'll need to look into this. I'll take a look and get back!

[pearcetm]

The current approach works better with explicitly modular JS, where scripts have explicit boundaries and have to be predictable - so they don't alter objects in the global namespace. While the module still extends OpenSeadragon, it can't make assumptions about OSD being in the client's global namespace and must have it passed down explicitly. It also can't access OSD using window.OpenSeadragon, since it can't assume that OSD is in the global namespace.

I am definitely in favor of explicitly modular JS in most circumstances! The reason I took the other approach (modifying the global OpenSeadragon object) was to follow the pattern used by most of the existing OSD plugins. For example, you can see the readme for the annotations plugin and for annotorious which shows accessing these plugins on the global OpenSeadragon object.

What I could do for backwards compat, is have an IIFE file that calls the current enableGeoTIFFTileSource on the window.OpenSeadragon object if present. But this would only work if OSD was also imported in a HTML script tag, and will not work if OSD is installed through NPM and imported in a module.

This approach sounds good to me - it is fine that it would only work as a plain script tag, many people use OpenSeadragon that way.

For the non minified and the source map, I'll need to look into this. I'll take a look and get back!

Awesome, thanks for looking into it!

[abiswas97]

Annotorious is actually where I got the idea! I used to use the openseadragon-annotation plugin in a non react project, and moved to annotorious when I started the react project. I didn't realise the annotorious github didn't mention their modular version. Their OpenSeadragon docs has a with npm section that passes the OpenSeadragon viewer instance to annotorious. I used this as my starting point for what the npm version of geotiff tilesource could be.

I'll check their codebase for how they maintain both the IIFE and ES module versions

[abiswas97]

If I'm understanding the Annotorious code right, it's never actually modifying OpenSeadragon from an IIFE.

    lib: {
      entry: path.resolve(__dirname, 'src/index.jsx'),
      name: 'OpenSeadragon.Annotorious',
      formats: [ 'umd' ]
    },

It's instead assigning the default exported react component to the global name OpenSeadragon.Annotorious. I don't think it's actually modifiying OpenSeadragon at all - OpenSeadragon.Annotorious will be created in the global namespace even if OpenSeadragon doesn't exist in the global namespace.

I tested this by including annotorious.min.js in an html page without OpenSeadragon - it doesn't throw any errors, and shows up as window.OpenSeadragon.Annotorious

This might not be a viable approach for this library though, since it does require access to the true OpenSeadragon namespace.

[abiswas97]

My current change adds in source maps (I couldn't find a clear approach to including both minified and unminified from one vite build command), and I've renamed the UMD output to explicitly be min.js.

For the IIFE, I'm not too sure how to proceed. While I could create a new file for it, I'd have to build it separately. Can we assume that if OpenSeadragon is available in global scope, we want to auto attach?

[abiswas97]

I think I have gotten a hacky auto-attach mechanism to work, but it makes assumptions

UMD

• If OpenSeadragon is imported as a script, plugin will auto-attach
• If OpenSeadragon is imported through npm, needs explicit call to enableGeoTIFFTileSource

ESM

• Needs explicit call to enableGeoTIFFTileSource

[abiswas97]

This change should allow for the UMD script to auto attach the TileSource to the global OSD. I've tested on the demo files, and edited the demos and README to reflect this.

Not sure if this was the right approach though - what do you think?

[pearcetm]

I don't think it's actually modifiying OpenSeadragon at all - OpenSeadragon.Annotorious will be created in the global namespace even if OpenSeadragon doesn't exist in the global namespace.

I'll look into this further, but my guess would be it attaches Annotorious to OpenSeadragon if it exists, and if OpenSeadragon doesn't exist in global scope, just creates an empty object.

[pearcetm]

Can we assume that if OpenSeadragon is available in global scope, we want to auto attach?

Yes, I think this makes sense. Something like

if(typeof window !== 'undefined' && window.OpenSeadragon){
    window.OpenSeadragon.GeoTIFFTileSource = ...
}

[pearcetm]

This change should allow for the UMD script to auto attach the TileSource to the global OSD. I've tested on the demo files, and edited the demos and README to reflect this.

Not sure if this was the right approach though - what do you think?

I'll take a look when I have a few minutes. Haven't had a chance yet.

[abiswas97]

Can we assume that if OpenSeadragon is available in global scope, we want to auto attach?

Yes, I think this makes sense. Something like

if(typeof window !== 'undefined' && window.OpenSeadragon){
    window.OpenSeadragon.GeoTIFFTileSource = ...
}

Awesome, this is very similar to the new commit! I've also added a check for whether it's running in an ES module environment, so that we never auto-patch while in ESM

[pearcetm]

Once this is merged, what's the process? What do I need to do to configure this to run the deployment script correctly? I imagine this will happen when creating a release - is that right?

[abiswas97]

Yup! You'll need to get an access token from npm and add it to the Github secrets for the repo. The github action is already set up to read from a secret called NPM_TOKEN. Once you manually create a tag for the current version and a release, the action should kick in and deploy to npm.

Just in case though, I'll list the full process I've followed so far with links. This the main guide I've used.

1. Create an npm token - the URL should be something like https://www.npmjs.com/settings/pearcetm/tokens/new . I use the classic token, with the type set to Automation.

   

2. Add the secret to this repo's secrets page as NPM_TOKEN.

3. Create a release from the repo's releases page. You'll need to create a new tag, something like v0.2.0, and assign it to the release. Once the release is published, you should be able to track the action.

Let me know if there are any issues setting this up / any issues with the automation script

[swamidass]

It would be better if there was just a function to call that installs GeoTiffTileSource

[abiswas97]

@swamidass do you mean like the enableGeoTIFFTileSource function? This will be available too - the function can be imported and manually called when using the npm package, but auto-installs when using the UMD script

[pearcetm]

@swamidass Both approaches will be available. Using it as a module you can attach it via the function call that @abiswas97 created and mentioned. Directly attaching to the OpenSeadragon global object when the plugin is included via a plain script tag is in keeping with the behavior of many existing OpenSeadragon plugins, so I think that is desirable too.

[pearcetm]

I've gone ahead and merged this. Thanks for making it happen, @abiswas97! If there are further changes needed we can address those in subsequent PRs.

[abiswas97]

Awesome!! Were you able to get the npm publishing script to work?

[pearcetm]

I haven't created a release yet. Since this was previously tagged as 1.0 I think it makes sense for the next release to be 2.0.0 (rather than 0.2.0), otherwise 1.0 would look like it is the latest... do you agree?

[abiswas97]

Yup that would make sense! The number in the package.json was for the fork - I forgot to align it with the original in this PR

[pearcetm]

@abiswas97 Ok, I've updated that.

I just noticed that geotiff.js is being bundled with the script - which is good - but it is difficult to find the license info for the bundled scripts. Do you know if there's a way in Vite to make the licenses get put into an easy-to-view place somehow?

[abiswas97]

Hmm I'm not sure - taking a look

[abiswas97]

@pearcetm I've found a plugin that might be doing exactly this. I've opened #12 with the changes

[swamidass]

So is this in npm yet? That would be great! An example of how to import the NPM module would be helpful too.

[pearcetm]

So is this in npm yet? That would be great! An example of how to import the NPM module would be helpful too.

@swamidass I believe I just managed to get it to deploy to NPM! As far as how to import it once you've installed from NPM, here's what @abiswas97 put into README.md as part of this major patch that enabled deploying to NPM:

import OpenSeadragon from 'openseadragon';
import { enableGeoTIFFTileSource } from "geotiff-tilesource";

enableGeoTIFFTileSource(OpenSeadragon);

Hopefully this works! If not, I'm hoping that @abiswas97 can help look into what we need to do to fix it.

[abiswas97]

This is awesome!! Really glad this all worked. I'll be available for any issues with the install steps.

@pearcetm Are you open to supporting other TIFF based formats through this plugin? We've been encountering somewhat specialised TIFF based bio-image formats at work, and I was planning on including them in my fork as and when I come across them.

If there are any other formats like QPTIFF that you'd want to support, I'd be happy to look into them and see what I can do

[swamidass]

I support better specialization on TIFF based bio-image formats!

[pearcetm]

Are you open to supporting other TIFF based formats through this plugin?

Yes, absolutely @abiswas97! I'd be happy to include support for those, and I welcome your contributions. Opening a PR is never a bad thing!