TS and JS build files of npm package out of sync about basemaps HOT 19 CLOSED

I've got the same issue in Bun. Using alpha.6 I can fix it with the following exports property

  "exports": {
    "bun": "./dist/index.mjs",
    "node": "./dist/index.cjs",
    "require": "./dist/index.cjs",
    "import": "./dist/index.mjs",
    "default": "./dist/index.cjs"
  },

These would be my suggestion to the package.json on the next release

from basemaps.

Can you try v2.0.0 that I just pushed?

This fixes exports to point at the ESM module and not the IIFE one, which is consistent with other packages. We don't support CJS so we don't need conditional exports.

I don't have enough information to reproduce the exact environments mentioned above so if you still have issue please push a minimal repo with Bun or Nuxt to test.

from basemaps.

Can you try 2.0.0-alpha.6?

I made some fixes to package.json, it might have been picking up an older CJS file by default

https://unpkg.com/browse/[email protected]/dist/index.js linę 1941 should be fixed
publint reports no issues: https://publint.dev/[email protected]

from basemaps.

Thanks so much for the quick response. But somehow this gives me different issues. If I just swap [email protected] with [email protected] I get the following error:

$ bun run src/index.ts
1 | (function (entry, fetcher)
    ^
SyntaxError: Missing 'default' export in module '{...}/node_modules/protomaps-themes-base/dist/index.js'.

My project runs in bun, but I also tested it with an empty Node project.

I suppose that it must be something about the changes in the exports part of the package.json. If I modify "exports": "./dist/index.js" with "exports": "./src/index.ts" or "exports": "./dist/index.mjs" inside the node_modules' package folder, everything seems to work.

Using "exports": "./dist/index.cjs" gives me another error

17 |   "light": layers("protomaps","light"),
                ^
TypeError: layers is not a function. (In 'layers("protomaps", "light")', 'layers' is an instance of Object)

Now I'm not an esbuild expert but I assume that with the new configuration my project picks up the compiled index.js file that is intended to run in the browser and there doesn't seem to be a way to tell it to do something else instead?

Thanks again!

from basemaps.

@Edefritz are you using pmtiles npm package in your project? the current exports key is defined the same way: https://github.com/protomaps/PMTiles/blob/main/js/package.json

from basemaps.

also am I correct in interpreting that this issue right now affects Bun and Bun only?

from basemaps.

Yes, I am using the npm package. But I tried editing some files manually to figure out what's going wrong. And no, I don't think it only affects bun. As I mentioned, I made a test with an empty node project as well and received the same error.

Perhaps the issue with the current export is that the .js build is created with the --format=iife flag. As far as I understand this optimises the output to be used in the browser but at the same time complicates usage in backend JS.
If I use the --format=esm flag instead and rebuild the js file, everything seems to work fine.

I think @eknowles' suggestion to use conditional exports makes sense.

from basemaps.

Just to confirm -> getting the same issue when trying to import in Nuxt app. (yarn)

import baseTheme from 'protomaps-themes-base';

Uncaught SyntaxError: The requested module '/_nuxt/node_modules/.cache/vite/client/deps/protomaps-themes-base.js?v=110663e5' does not provide an export named 'default' (at mapBaseThemeLoader.ts:1:8)

from basemaps.

Great, that fixes it for me. Thanks so much! :)

from basemaps.

We don't support CJS so we don't need conditional exports.

why the change of heart here @bdon? Up until 2.0.0alpha5 my project build correctly. Now I have to move an entire monorepo to ESM to build this project. :|

from basemaps.

@iwpnd the feedback I got in other issues (protomaps/PMTiles#287) as well as docs like https://nodejs.org/api/packages.html#dual-package-hazard was that we should prefer moving to ESM-only; for this case in particular is there a workaround, like using the generated style JSON instead of depending on the NPM package?

from basemaps.

Also making it an ESM only package is desirable now to encourage people to stop using cjs as it's a constant burden on the ecosystem. Instead of us transpiring it, let them do it, ESM has support as far back as node 12, we should only be supporting 18, and soon 20 (current LTS). (protomaps/PMTiles#287)

That's a pretty extrem view. One that is not shared with a lot of other libraries of higher popularity in the ecosystem. I don't think you have educational duty to enforce, nor do I believe you want to. 😄
As a maintainer however I can understand the decision, as much as I dislike it.

for this case in particular is there a workaround, like using the generated style JSON instead of depending on the NPM package?

I believe you went the same route with the pmtiles package, so at the latest it'll bite me there.

from basemaps.

That's a pretty extrem view. One that is not shared with a lot of other libraries of higher popularity in the ecosystem.

Fair! I wasn't aware of CJS projects using PMTiles, but I think CJS is important for migrating legacy projects to PMTiles if they're depending on a deprecated 3rd party API, etc.

Is your CJS project a command-line NodeJS one? Trying to understand the background here. Since this project and the other JS ones (pmtiles, protomaps-leaflet) are all bought into ESBuild, we could add CJS support back in via a pmtiles.cjs build with a conditional export in package.json pretty easily; the CJS build was previously the default, and now the ESM one is.

from basemaps.

Is your CJS project a command-line NodeJS one?

It's an express backend that uses pmtiles and this package to provide a basemap with different styling options. Since it has a bunch of dependencies with other parts of the monorepo, i'd have to move the entire thing. Again, that's not your problem, so I understand if you'd not like to provide CJS support. But I'd appreciate it nonetheless. :)

Since this project and the other JS ones (pmtiles, protomaps-leaflet) are all bought into ESBuild, we could add CJS support back in via a pmtiles.cjs build with a conditional export in package.json pretty easily; the CJS build was previously the default, and now the ESM one is.

This sounds perfectly fine. 👍

from basemaps.

Proposed change and relevant TypeScript issue that arises: #248 (comment)

I don't know how much of a dealbreaker this is.

from basemaps.

Can't tell you without testing it, i think.

from basemaps.

Try 3.0.1 https://www.npmjs.com/package/protomaps-themes-base ?

from basemaps.

My bad, wasn’t aware that you already released

from basemaps.

Hey @bdon
Sorry to get back to you this late.

In short, it doesn't work for me. Importing either pmtiles or protomaps-themes-base will result in the expected:

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("pmtiles")' call instead.                                            To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '[...]/package.json'.

Or

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("protomaps-themes-base")' call instead.                                            To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '[...]/package.json'.

respectively, because the default is ESM.

I can use e.g.

import { labels, noLabels } from 'protomaps-themes-base/require';

on import, but it would complain about missing type definitions, something I believe you pointed out in #248.

from basemaps.