GeoTIFF/georaster-layer-for-leaflet

Rollup build produces a runtime error

luka-mikec opened this issue · 11 comments

luka-mikec commented

Describe the bug
When building an NPM project using Vite/Rollup, an error is produced during runtime (something like xyz.defs is not a function with a random string in place of xyz).

Edit: turned off minification to see what the xyz is: proj4$1.defs(defs);

To Reproduce
Steps to reproduce the behavior:

  1. Create a new vanilla JavaScript Vite project with npm init vite, and name it e.g. georaster-layer-for-leaflet-example
  2. cd georaster-layer-for-leaflet-example
  3. npm install georaster-layer-for-leaflet
  4. At the top of main.js add: import 'georaster-layer-for-leaflet'
  5. npm run build && npm run preview
  6. Open http://localhost:4173/ in a browser; an error will be thrown in the console.

Expected behavior
No error should appear in the console.

luka-mikec commented

After more debugging I found that the issue is caused by a check within the proj4-fully-loaded dependency, specifically this block:

if (typeof proj4 === "object" && typeof proj4.defs !== "function" && typeof proj4.default === "function") {
  // probably inside an Angular project
  proj4 = proj4.default;
}

More specifically, the first operand (typeof proj4 === "object" ) is false in my case (the expression returns a function, so typeof proj4 is "function"). The other two conditions pass. I submitted a possible fix (removal of the first operand/equality check) in a PR in that library's repo: DanielJDufour/proj4-fully-loaded#3

The error disappeared after I manually changed this check in the following files:

./node_modules/georaster-layer-for-leaflet/dist/georaster-layer-for-leaflet.min.js
./node_modules/georaster-layer-for-leaflet/dist/v3/webpack/bundle/georaster-layer-for-leaflet.min.js
./node_modules/reproject-geojson/node_modules/proj4-fully-loaded/proj4-fully-loaded.js
./node_modules/reproject-geojson/reproject-geojson.min.js
./node_modules/geocanvas/dist/geocanvas.min.js
./node_modules/proj4-fully-loaded/proj4-fully-loaded.js
./node_modules/reproject-bbox/reproject-bbox.min.js

Unfortunately I cannot persist these changes with patch-package since the change involves a transitive dependency which is not a peer dependency (my project -> georaster-layer-for-leaflet -> proj4-fully-loaded) and by the time postinstall scripts are run, these transitive dependency has already been installed. I could write a postinstall script that would do the thing I did manually, but it's tricky since it depends on non-deterministic minifying code.

DanielJDufour commented

Hey! Just wanted to let you know I'm reading this. I really appreciate the time you've taken diving into the details. I'm overloaded with work and family events the next few days, but should have time next week to review. Thank you! It's really impressive you were able to dig through the libraries so quickly!

  • 👍1
luka-mikec commented

Hi @DanielJDufour, did you have a chance to review the PR (DanielJDufour/proj4-fully-loaded#3) by any chance? I wrote an explanation there of why that PR does not have any negative side effects, i.e. there is no situation where the PR's behaviour would cause an error and the existing code would not, so it should be safe to merge.

wrengames commented

I am also waiting for this fix and wondered if you had any news @luka-mikec @DanielJDufour :)

luka-mikec commented

@wrengames It seems Daniel is busy :), but I found out that recent versions of npm support changing dependencies' dependencies through a special overrides property, so a relatively simple workaround is:

  1. Make a local copy of proj4-fully-loaded (e.g. proj4-fully-loaded-patched) and change the JavaScript file's code to (if you only need Rollup): 
    import proj4 from "proj4";
import * as defs from "proj4js-definitions";

proj4.defs(defs);

export default proj4;
export {proj4};
  2. Find out which packages depend on proj4-fully-loaded with npm list --depth=100
  3. Set those packages' version to be the local copy in package.json. Here is what I needed to set: 
    {
  (...)
  "overrides": {
    "georaster-layer-for-leaflet": {
      "proj4-fully-loaded": "file:proj4-fully-loaded-patched",
      "reproject-bbox": {
        "proj4-fully-loaded": "file:proj4-fully-loaded-patched"
      },
      "geocanvas": {
        "geomask": {
          "reproject-geojson": {
            "proj4-fully-loaded": "file:proj4-fully-loaded-patched"
          }
        }
      }
    }
  }
}
  • ❤️1
DanielJDufour commented

you're awesome @luka-mikec ! sorry for the late reply. I'll take a deep look when I metaphorically come up for air. In the meantime, would it be possible to add a test that captures this rollup issue to https://github.com/GeoTIFF/georaster-layer-for-leaflet/tree/master/tests similar to https://github.com/GeoTIFF/georaster-layer-for-leaflet/tree/master/tests/vue. It'll make accepting and testing faster (and making sure no regressions happen in the future).

thank you!!

wrengames commented

@wrengames It seems Daniel is busy :), but I found out that recent versions of npm support changing dependencies' dependencies through a special overrides property, so a relatively simple workaround is:

  1. Make a local copy of proj4-fully-loaded (e.g. proj4-fully-loaded-patched) and change the JavaScript file's code to (if you only need Rollup): 
    import proj4 from "proj4";
import * as defs from "proj4js-definitions";

proj4.defs(defs);

export default proj4;
export {proj4};
  2. Find out which packages depend on proj4-fully-loaded with npm list --depth=100
  3. Set those packages' version to be the local copy in package.json. Here is what I needed to set: 
    {
  (...)
  "overrides": {
    "georaster-layer-for-leaflet": {
      "proj4-fully-loaded": "file:proj4-fully-loaded-patched",
      "reproject-bbox": {
        "proj4-fully-loaded": "file:proj4-fully-loaded-patched"
      },
      "geocanvas": {
        "geomask": {
          "reproject-geojson": {
            "proj4-fully-loaded": "file:proj4-fully-loaded-patched"
          }
        }
      }
    }
  }
}

Thanks for this @luka-mikec ! You have saved me a load of work finding a replacement for the georaster plugin.

Thanks also to @DanielJDufour for making the plug-in on the first place 😊

DanielJDufour commented

I updated the dependencies with the fix, but I'm in the middle of another update, so I'm going to combine the work and try to get a new version published later this week. (I'll save time that way)

I created a new repo: https://github.com/GeoTIFF/georaster-layer-for-leaflet-vite and invited you both as maintainers. We can use it to test the new version of georaster-layer-for-leaflet once it is released.

luka-mikec commented

@wrengames np!

@DanielJDufour thanks! The version 0.2.0 doesn't cause an issue: GeoTIFF/georaster-layer-for-leaflet-vite#1
I don't know if you wanted to merge this workaround or propagate the new version of proj4-fully-loaded through other dependencies (reproject-bbox and geocanvas -> geomask -> reproject-geojson)? If propagate, we don't need to change anything in that repo except maybe the version of georaster-layer-for-leaflet (to be the future one).

Regarding tests, I can add it there, did you mean to add just the basic example without workarounds (the one that will work once the new version propagates everywhere)?

  • ❤️1
wrengames commented

Geoblaze also needs the dependency updated for my usage please. Although I can put an override to this new version in my app 😊

Zwabo commented

Had the same problem - Updating the dependency to 0.2.0 via the overrides option also solved it for me. Thank you @luka-mikec! Would love to see this fixed asap, took me some hours to figure out :D