protomaps / protomaps-leaflet

Lightweight vector map rendering + labeling and symbology for Leaflet
https://protomaps.com/docs/frontends/leaflet
BSD 3-Clause "New" or "Revised" License
755 stars 43 forks source link

ESM build target #162

Closed prushforth closed 2 months ago

prushforth commented 2 months ago

I noticed that the dist/index.js is not actually the output of the build-module script (is it supposed to be?). Perhaps if the output of that script was available on CDN it would allow use via <script type=module src="https://unpkg.com/protomaps-leaflet@latest/dist/index.js"></script>

THANK YOU for an amazing project!

bdon commented 2 months ago

You're right, the build-tsc task is clobbering the other files. I will need to push a new major release that will change the module paths, probably adding a esm/ folder to the distribution. PR coming.

bdon commented 2 months ago

Can you please try 4.0.0-alpha.0

prushforth commented 2 months ago

Can you please try 4.0.0-alpha.0

I'm using grunt-rollup. I reference the esm in my index.js for input to rollup like so:

import { leafletLayer } from '../../node_modules/protomaps-leaflet/dist/esm/index.js';
import {  LineSymbolizer,  PolygonSymbolizer} from '../../node_modules/protomaps-leaflet/dist/esm/index.js';

When I run the rollup, I get a few issues:

Running "rollup:main" (rollup) task
'@mapbox/point-geometry' is imported by node_modules/protomaps-leaflet/dist/esm/index.js, but could not be resolved – treating it as an external dependency
'color2k' is imported by node_modules/protomaps-leaflet/dist/esm/index.js, but could not be resolved – treating it as an external dependency
'@mapbox/vector-tile' is imported by node_modules/protomaps-leaflet/dist/esm/index.js, but could not be resolved – treating it as an external dependency
'pbf' is imported by node_modules/protomaps-leaflet/dist/esm/index.js, but could not be resolved – treating it as an external dependency
'pmtiles' is imported by node_modules/protomaps-leaflet/dist/esm/index.js, but could not be resolved – treating it as an external dependency
'rbush' is imported by node_modules/protomaps-leaflet/dist/esm/index.js, but could not be resolved – treating it as an external dependency
'potpack' is imported by node_modules/protomaps-leaflet/dist/esm/index.js, but could not be resolved – treating it as an external dependency
No name was provided for external module '@mapbox/point-geometry' in output.globals – guessing 'X'
No name was provided for external module 'color2k' in output.globals – guessing 'color2k'
No name was provided for external module '@mapbox/vector-tile' in output.globals – guessing 'vectorTile'
No name was provided for external module 'pbf' in output.globals – guessing '_t'
No name was provided for external module 'pmtiles' in output.globals – guessing 'pmtiles'
No name was provided for external module 'rbush' in output.globals – guessing 'Et'
No name was provided for external module 'potpack' in output.globals – guessing 'qt'

When I load the test page I see a console error log:

Uncaught ReferenceError: X is not defined
    at mapml.js:10943:3

The technique I was using up to now was to run your npm run build-module and to import those symbols from the index.js created.

bdon commented 2 months ago

Can you upload your full project so others can reproduce?

prushforth commented 2 months ago

The branch I have been experimenting with is on 3.1.2, I changed that to depend on 4.0.0-alpha0 in this branch. In there, the file src/mapml/index.js imports directly from the node_modules-installed esm/index.js. Maybe I'm doing that step wrong. Probably!

prushforth commented 2 months ago

I tried changing the import to import * as protomapsL from "..\..\node_modules\protomaps-leaflet\dist\esm\index.js" and then defining my own references to protomapsL.leafletLayer etc., but the result is the same.

I think that while the protomaps-leaflet dependencies are installed in my node_modules, for some reason the importing being done by "....\node_modules\protomaps-leaflet\dist\esm\index.js" is not finding the various packages i.e. this message

'@mapbox/point-geometry' is imported by node_modules\protomaps-leaflet\dist\esm\index.js, but could not be resolved – treating it as an external dependency

is a clue. Maybe it's the grunt-rollup plugin I'm using. I'll try updating that.

prushforth commented 2 months ago

I added the \@rollup/plugin-node-resolve plugin to my grunt-rollup plugin, and most of the module resolving errors went away, but it still doesn't complete the build, finishing with this message:

Running "rollup:main" (rollup) task
Warning: 'default' is not exported by node_modules/@mapbox/point-geometry/index.js, imported by node_modules/protomaps-leaflet/dist/esm/index.js Use --force to continue.

Aborted due to warnings.
Done.

If all of those modules could be bundled into the protomaps-leaflet/dist/esm/index.js for release, I think it might simplify usage of the release artifact. But it might be a big file. I wonder if tsup will do that for you?

bdon commented 2 months ago

Are the initial ESM issues unique to this package, or are you successfully using other ESM packages that aren't bundled?

I will take a look at the point-geometry issue, thanks.

prushforth commented 2 months ago

Are the initial ESM issues unique to this package, or are you successfully using other ESM packages that aren't bundled?

I don't believe any of the dependencies we currently have have any dependencies of their own, so bundling has not come up yet. We have forked code (and licenses) from some projects directly into our project, so deps of deps hasn't been an issue so far. I noticed that the build-module task at protomaps-leaflet 3.1.2 produces an index file that doesn't have this problem, and maybe the --bundle option is not being passed to tsup.

bdon commented 2 months ago

Can you please try again with 4.0.0-alpha.1

bdon commented 2 months ago

maybe the --bundle option is not being passed to tsup.

Other users of the library prefer that the library does not bundle its dependencies, except in the script-includes case which need to bundle to work. So this 4.0.0 will change the ESM and CJS output to unbundled.

prushforth commented 2 months ago

Can you please try again with 4.0.0-alpha.1

That seems to sove all the missing symbol problems, using \@rollup/plugin-node-resolve now. Thanks!

Other users of the library prefer that the library does not bundle its dependencies, except in the script-includes case which need to bundle to work. So this 4.0.0 will change the ESM and CJS output to unbundled.

OK, maybe we're missing out on some benefit of unbundled code. Will investigate.

ICYMI I hadn't tried this library up until now because the examples all depend on Leaflet 1.7, and we are using 1.9.4 and I believed that protomaps-leaflet wouldn't work with the more recent version. I don't know why I tried it more recently, but it seems to work fine. Or maybe I tried it some time ago and it didn't work at that time, I don't recall. I noticed that some good guy with a PC has registered typescript definitions for a more recent Leaflet, so maybe that's what makes the difference. If there's a maintenance function here that you can request of the community, I would be interested to help.

bdon commented 2 months ago

v4.0.0 has been published https://www.npmjs.com/package/protomaps-leaflet

please reopen if you encounter any new build issues.