Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESM build target #162

Closed
prushforth opened this issue Jul 24, 2024 · 13 comments
Closed

ESM build target #162

prushforth opened this issue Jul 24, 2024 · 13 comments

Comments

@prushforth
Copy link

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
Copy link
Member

bdon commented Jul 25, 2024

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 added a commit that referenced this issue Jul 25, 2024
* change IIFE name from protomaps-leaflet.js to index.global.js
* change default export for theme.ts to named export
@bdon
Copy link
Member

bdon commented Jul 25, 2024

Can you please try 4.0.0-alpha.0

@prushforth
Copy link
Author

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
Copy link
Member

bdon commented Jul 26, 2024

Can you upload your full project so others can reproduce?

@prushforth
Copy link
Author

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
Copy link
Author

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
Copy link
Author

prushforth commented Jul 26, 2024

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
Copy link
Member

bdon commented Jul 26, 2024

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
Copy link
Author

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
Copy link
Member

bdon commented Jul 28, 2024

Can you please try again with 4.0.0-alpha.1

@bdon
Copy link
Member

bdon commented Jul 28, 2024

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
Copy link
Author

prushforth commented Jul 28, 2024

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 added a commit that referenced this issue Jul 28, 2024
* Change to tsup for bundling [#158,#161,#162]
* change default export for theme.ts to named export
* update vector-tile, pbf and point-geometry dependencies [#162]
* 4.0.0
@bdon
Copy link
Member

bdon commented Jul 28, 2024

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

please reopen if you encounter any new build issues.

@bdon bdon closed this as completed Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants