TS and JS build files of npm package out of sync

[Edefritz]

Hi,

I discovered something unusual. When installing the latest protomaps-themes-base@2.0.0-alpha.5, I expected that the improvement I suggested a while ago would be reflected.

According to the base_layers.ts it is. But checking the style after I updated the package in my code base, the output seemed exactly the same as before.

After a lot of back and forth debugging, I found that the compiled .js, .mjs and .cjs files in the node_modules folder, didn't seem to have been updated.

To rule out issues on my local installation, I checked the source of the package inside NPM: https://www.npmjs.com/package/protomaps-themes-base?activeTab=code

As you can see in the screenshots the contents of the JS and TS files don't match (zoom should be 6 in line 1927).

I'm not sure this is how it works but perhaps you didn't compile the files before publishing the package? Or I'm awfully lost 😄

[bdon]

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/protomaps-themes-base@2.0.0-alpha.6/dist/index.js linę 1941 should be fixed publint reports no issues: https://publint.dev/protomaps-themes-base@2.0.0-alpha.6

[Edefritz]

Thanks so much for the quick response. But somehow this gives me different issues. If I just swap protomaps-themes-base@2.0.0-alpha.5 with protomaps-themes-base@2.0.0-alpha.6 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!

[eknowles]

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

[bdon]

@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

[bdon]

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

[Edefritz]

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.

[aptum11]

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)

[bdon]

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.

[Edefritz]

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

[iwpnd]

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. :|

[bdon]

@iwpnd the feedback I got in other issues (https://github.com/protomaps/PMTiles/issues/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?

[iwpnd]

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). (https://github.com/protomaps/PMTiles/issues/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.

[bdon]

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.

[iwpnd]

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. 👍

[bdon]

Proposed change and relevant TypeScript issue that arises: https://github.com/protomaps/basemaps/pull/248#issuecomment-2109355625

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

[iwpnd]

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

[bdon]

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

[iwpnd]

My bad, wasn’t aware that you already released

[iwpnd]

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.

[bdon]

I would like to create a complete, runnable example that demonstrates the issue, if not a runnable project at least:

1) build tool or bundler 2) package.json 3) typescript configuration

At least var themes = require('protomaps-themes-base'); works on 3.0.1 and not 3.0.0

[iwpnd]

so I created this:https://github.com/iwpnd/pmtiles-example-ts

using a tsconfig like the above the projects builds no problem.

The problem arises is when I change to:

    "module": "nodenext",
    "moduleResolution": "nodenext"

I was able to build like so prior to v3, now this doesn't build anymore.

Now I have to admit that I'm lacking some background on how all of this ties together I'm afraid. From my point of view something changed with v3 that would not allow my build to succeed.

[bdon]

So your project uses nodenext already and using "module": "commonjs", "moduleResolution": "node" is not an option?

[bdon]

What if you use:

const themesBase = require('protomaps-themes-base');
console.log(themesBase.labels, themesBase.noLabels);

[iwpnd]

So your project uses nodenext already and using "module": "commonjs", "moduleResolution": "node" is not an option?

that is correct. also, while it may build, it doesn't run because of

// node dist/index.js
const pmtiles_1 = require("pmtiles");
                  ^
Error [ERR_REQUIRE_ESM]: require() of ES Module

What if you use:

aside from mixing require and es import syntax is urgh, it works, yet there's no types.

I don't yet quite understand why you drop the behaviour prior to pmtiles v3/protomaps-base-themes v2alpha5, when now we try to revert and support commonjs. Shouldn't it be straight forward to revert all together and return to the previous build behaviour if going forward you are open to support commonjs and esm modules?

[bdon]

const pmtiles_1 = require("pmtiles");

This is because I have not added the conditional export for require to the pmtiles package yet. If this fallback for protomaps-themes-base is working then I can go ahead with that.

I don't yet quite understand why you drop the behaviour prior to pmtiles v3/protomaps-base-themes v2alpha5, when now we try to revert and support commonjs. Shouldn't it be straight forward to revert all together and return to the previous build behaviour if going forward you are open to support commonjs and esm modules?

Because pre 3.0.0 it was default CommonJS (no type:module in package.json: https://github.com/protomaps/basemaps/commit/4329cfb9eb9b947ed980e8b8ef713c58b2dca7f2) , so even ESM projects were getting the CommonJS build and causing problems. It seems proper that ESM is the default in 2024 and we want a fallback now.

[iwpnd]

Because pre 3.0.0 it was default CommonJS (no type:module in package.json: https://github.com/protomaps/basemaps/commit/4329cfb9eb9b947ed980e8b8ef713c58b2dca7f2)

gotcha, makes sense. 👍

so even ESM projects were getting the CommonJS build and causing problems

This is not supposed to be a problem. The other way around is however. 🤔

It seems proper that ESM is the default in 2024 and we want a fallback now.

somewhat understandable, definitely debatable - but in the end the maintainers decision.

Anyhow, I'd be happy to test anything you want me to test further. But also feel free dropping it entirely. Appears to be a me problem that I'll have to address on my end when the time comes.

[bdon]

@iwpnd you can use pmtiles v3.0.6 which is on npm now, thanks for being patient.

I don't understand why your project used import successfully with the CommonJS build in the past - if I observe more CommonJS related issues then I may need to rework the build system to package this for both Node and browsers properly.

[iwpnd]

I don't understand why your project used import successfully with the CommonJS build in the past

It is actually the preferred way to write typescript imports and works just fine for as long as the underlying third party package is not an esm module. Only the resulting javascript will contain require calls, which is why it doesn't work for esm modules.

Thank you for spending time on this nonetheless. Much appreciated. 🙏

[iwpnd]

I just happen to stumble upon how turfjs handles commonjs/esm if you're interested, here. @bdon considering the popularity of turfjs, i reckon it's a good role model to follow?

[bdon]

Yes, thanks for the link. So it looks like at build time we need to create a separate set of .cjs and d.cts files. Looking in to how to do that...

[bdon]

It looks like Turf depends on adopting https://github.com/egoist/tsup along with its configuration https://github.com/Turfjs/turf/blob/09e9ebdfeef561ddfc67070064f6b8b72eedebd9/tsup.config.ts to generate dual packages

are there examples of other projects using tsup we can compare?

[bdon]

A good writeup in the README here https://github.com/isaacs/tshy even if we're not going to use tshy

[iwpnd]

are there examples of other projects using tsup we can compare?

vercel/ai faker-js huggingfacejs in every package of the monorepo

[bdon]

migrating https://github.com/protomaps/protomaps-leaflet/pull/163 to tsup first before I do protomaps-themes-base and finally pmtiles

[bdon]

@iwpnd can you try 3.1.0-alpha.0

[iwpnd]

Nice @bdon! The project builds! 🥳

[bdon]

3.1.0 is published