build(query-parser): replace gen-esm-wrapper with rollup COMPASS-10271

[rtritto]

Description

Replace gen-esm-wrapper with rollup

Related:

• https://jira.mongodb.org/browse/COMPASS-10271
• TypeError: toJSString is not a function nitedani/standaloner#8

Checklist

• I have signed the Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)
• New tests and/or benchmarks are included
• Documentation is changed or added

[rtritto]

Please update package-lock.json with npm i

FYI @addaleax

[addaleax]

As you can see, this PR isn't currently passing CI. I think rollup is having trouble with our typescript config?

In any case, I'm not sure how I feel about replacing our tooling because of rolldown not being able to handle CJS where Object.defineProperty() is used. That seems like a deficiency in rolldown, not in our tooling here?

[rtritto]

As you can see, this PR isn't currently passing CI. I think rollup is having trouble with our typescript config?

I know, the package-lock.json needs to be updated, I can't do because the scripts fail on my Windows machine. You can run npm i to update package-lock.json and then commit it.

In any case, I'm not sure how I feel about replacing our tooling because of rolldown not being able to handle CJS where Object.defineProperty() is used. That seems like a deficiency in rolldown, not in our tooling here?

Seems that tsc (CJS) adds Object.defineProperty and then gen-esm-wrapper adds the ESM wrapper.
Object.defineProperty should be used only with CJS and not with ESM: I have the issue with rollup (ESM) and Object.defineProperty.

With this PR, you can also check inside the dist folder generated by rollup:

• cjs/index.js has 1 Object.defineProperty
• esm/index.js hasn't Object.defineProperty

Actually Object.defineProperty was used only here, I don't know if this needs a refactoring.

We need two different builds (CJS and ESM) or only ESM (new versions of devtools-shared tools as breaking change).

---

We can use directly a build tool (gen-esm-wrapper will not be needed): I used rollup because it's used by bson, feel free to use other alternatives (rolldown, tsdown, esbuild, pkgroll etc).

[addaleax]

Object.defineProperty should be used only with CJS and not with ESM

Right, the problem with rollup is that Object.defineProperty(exports, ...) is only being used with CJS and not with ESM.

What rollup doesn't support is importing from an CJS module that uses this; but that's a deficiency in rollup (or at least I can't see why it wouldn't be – default imports from CJS should be something that bundlers support unambiguously).

Using split builds might be the way to go in the long run, but it has its non-trivial downsides (e.g. object identity changing between the builds) and I'm not sure that it's a road we'd want to go down without discussing it on the team first

[rtritto]

Using split builds might be the way to go in the long run, but it has its non-trivial downsides (e.g. object identity changing between the builds) and I'm not sure that it's a road we'd want to go down without discussing it on the team first

Please can a discussion/issue be opened?
This issue blocks all my projects.

[rtritto]

@kmruiz @Anemy @nbbeeken Please anyone can open a related discussion/issue?

[rtritto]

I created https://jira.mongodb.org/browse/COMPASS-10326

[kmruiz]

Hey @rtritto! Thanks for the PR and for opening the ticket.

We’re not planning to change the build setup in the short term. The issue this PR addresses is a Rollup limitation, and this proposal doesn't offer clear enough benefits to justify the effort right now. While we may revisit the build system in the future, gen-esm-wrapper does generate valid ESM that works with Node.js and other bundlers as expected

This repo is shared across all our tooling, so even small config changes can impact first-tier components like mongosh or Compass. Given that, the risk and validation effort currently outweigh the benefits.

From the linked issues it looks like the Rollup workaround unblocked you. Can you confirm that’s still the case?

[rtritto]

Hi @kmruiz, thanks for reply.

We’re not planning to change the build setup in the short term. The issue this PR addresses is a Rollup limitation, and this proposal doesn't offer clear enough benefits to justify the effort right now. While we may revisit the build system in the future, gen-esm-wrapper does generate valid ESM that works with Node.js and other bundlers as expected

This isn't only a rollup issue/limitation because other bundle tools will have same trouble.
Object.defineProperty can be used only with CJS and not with ESM: Object.defineProperty with ESM isn't a compliant/standard code.
So use a dual build (CJS + ESM) or a single build (ESM).

This repo is shared across all our tooling, so even small config changes can impact first-tier components like mongosh or Compass. Given that, the risk and validation effort currently outweigh the benefits.

The change must be made on all tools as a breaking change (increasing the major version number), by this way, nothing will break.
Please address this issue as well you think.

From the linked issues it looks like the Rollup workaround unblocked you. Can you confirm that’s still the case?

The workaround doen't work for me (I closed the issue because the problem was here and not on standaloner/rollup side).
I'm still blocked, I can't use mongo-query-parser on any project.
Any other user may have the same problem with other tools.

[kmruiz]

This isn't only a rollup issue/limitation because other bundle tools will have same trouble.
Object.defineProperty can be used only with CJS and not with ESM: Object.defineProperty with ESM isn't a compliant/standard code.
So use a dual build (CJS + ESM) or a single build (ESM).

From my understanding, that's not entirely correct. Both CJS and ESM support Object.defineProperty just fine, we are using webpack and everything is bundled accordingly. I understand that rollup does not detect this specific pattern, but other tools like webpack and Node.js through cjs-module-lexer work fine.

We might want to change in the future the build system, but as mentioned earlier, the current system works as expected for mongosh, Compass and other products we maintain without substantial issues.

The change must be made on all tools as a breaking change (increasing the major version number), by this way, nothing will break.
Please address this issue as well you think.

Currently we don't have the capacity to address this. Changing all our products, which are complex codebases with pretty advanced build systems is not something we are willing to do in the short term because of a limitation in rollup.

The workaround doen't work for me (I closed the nitedani/standaloner#8 because the problem was here and not on standaloner/rollup side).
I'm still blocked, I can't use mongo-query-parser on any project.
Any other user may have the same problem with other tools.

From what I see, standaloner is a native application packager. The only option we can give you at the moment is to use a native application packager that does not use rollup, so you can avoid this limitation. In our case, we use boxednode for mongosh because it supports native addons and it's bundler agnostic.