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

[commonjs] TypeScript compiled type-only modules render invalid bundles #743

Closed
willmtemple opened this issue Dec 21, 2020 · 8 comments
Closed

Comments

@willmtemple
Copy link

willmtemple commented Dec 21, 2020

I have some time/resources to take a crack at writing a patch for this if any maintainers can give me some pointers about where to go digging.

  • Rollup Plugin Name: commonjs
  • Rollup Plugin Version: ^17.1.0
  • Rollup Version: ^2.40.0
  • Operating System (or Browser): Building on Linux and Windows, testing in Chrome Headless
  • Node Version: 14.15.5
  • Link to reproduction (⚠️ read below): https://github.com/willmtemple/ot-commonjs-repro

Expected Behavior

Bundling the following code for use in browser testing should work as expected, or should throw an error/warning indicating that the bundle may have failed:

import { TraceFlags } from "@opentelemetry/api";

console.log(TraceFlags);

(TraceFlags is an enum.)

Actual Behavior

The bundle contains invalid references to exports in the inlined OpenTelemetry code, causing the bundle to throw:

  Uncaught ReferenceError: exports is not defined
  at dist-test/index.browser.js:298:24

  ReferenceError: exports is not defined
      at dist-test/index.browser.js:298:24
      at dist-test/index.browser.js:3:2
      at dist-test/index.browser.js:4:2

The stack trace refers to the line:

Object.defineProperty(exports, "__esModule", { value: true });

On this line, exports is not bound to any variable declaration. The module that generated this line comes from a type-only module, and this is the only line of code in the module (other than "use strict";).

Additional Information

I have provided a reproduction of this issue with some instructions at https://github.com/willmtemple/ot-commonjs-repro. UPDATE: This bug apparently occurs with any type only modules, so I renamed the issue accordingly, and I changed the repro repo to use an example dependency package that has only a type-only module. @opentelemetry/api just happens to be such a module.

The repro repo contains instructions and a few notes about things I've (unsuccessfully) tried. My guess is that this is tied up in the logic that plugin-commonjs uses to decide that the module is a "restorable" ESM using the __esModule sentinel.

niklasf added a commit to lichess-org/lila that referenced this issue Dec 29, 2020
The bundled round.nvui.js was failing with:

    round.nvui.js:816 Uncaught ReferenceError: exports is not defined

Around:

    Object.defineProperty(exports, "__esModule", { value: true });
ornicar added a commit to lichess-org/lila that referenced this issue Dec 29, 2020
* 'master' of github.com:ornicar/lila:
  downgrade @rollup/plugin-commonjs for rollup/plugins#743
  Revert "Revert "update typescript to 4.1.x (preparing chessops 0.8.1)""
  make https://lichess.org/fishnet a permalink
@niklasf
Copy link

niklasf commented Jan 25, 2021

This bug applies to IIFE bundles as well.

@willmtemple willmtemple changed the title [commonjs] Bundling @opentelemetry/api as UMD results in an invalid bundle [commonjs] TypeScript compiled type-only modules render invalid bundles Mar 1, 2021
@witemple-msft
Copy link

CC @bterlson

@shellscape
Copy link
Collaborator

We're using BuilderProgram so that must be outputting something that commonjs is picking up on. That's the only idea I have for you at the moment.

@witemple-msft
Copy link

@shellscape Did some digging through the code and found that this was fixed in #816. Is there a timeline for releasing this?

@shellscape
Copy link
Collaborator

At the moment plugin publishes are performed when we have a moment to do them. You'll notice by the commit history in the repo that we had a spate of them not too long ago, followed by a recent merge party of PRs. In the future we might implement a more on-demand publishing setup, but right now we do some auditing for each version. Consult the commit history for typical times between merge and publish.

@benduran
Copy link

benduran commented Apr 5, 2021

Subscribed for updates. Trying to migrate a multi-application monorepo away from CRA to using Vite, and this is a blocker for us at the moment 🤕 (since Vite is using Rollup under the hood)

@stale
Copy link

stale bot commented Jun 6, 2021

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

@stale stale bot closed this as completed Jun 6, 2021
@canadaduane
Copy link

This was fixed by #816 (I'm not sure what version of @rollup/plugin-commonjs that was merged in to, but I've tested 21.0.1 and it works).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants