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

feat: add cjsInterop support without splitting flag #1056

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tmkx
Copy link
Contributor

@tmkx tmkx commented Dec 8, 2023

esbuild does not provide an exports metadata when the format is cjs.

splitting is working because it's also bundled as esm

tsup/src/esbuild/index.ts

Lines 164 to 165 in fb4c2b6

format:
(format === 'cjs' && splitting) || options.treeshake ? 'esm' : format,

and then transformed to cjs:
const { transform } = await import('sucrase')
const result = transform(code, {
filePath: info.path,
transforms: ['imports'],
sourceMapOptions: this.options.sourcemap
? {
compiledFilename: info.path,
}
: undefined,
})

Copy link

codesandbox bot commented Dec 8, 2023

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

vercel bot commented Dec 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tsup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2024 9:02am

@kibertoad
Copy link

@sxzz Aby updates on this?

src/plugin.ts Outdated Show resolved Hide resolved
)
}

let entrySource: string | undefined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't use code param directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code has already been transpiled to cjs, but we need the source code here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could break the plugin system. If a plugin attaches a named export, then cjs interop cannot analyze it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, but cjs is difficult to analyze exports, and esbuild does not provide output meta either when the format is cjs

src/plugins/cjs-interop.ts Outdated Show resolved Hide resolved
src/plugins/cjs-interop.ts Outdated Show resolved Hide resolved
src/plugins/cjs-interop.ts Show resolved Hide resolved
src/plugins/cjs-interop.ts Show resolved Hide resolved
src/plugins/cjs-interop.ts Outdated Show resolved Hide resolved
src/plugins/cjs-interop.ts Outdated Show resolved Hide resolved
src/plugins/cjs-interop.ts Outdated Show resolved Hide resolved
...(filename.endsWith('.jsx') ? { jsx: true } : null),
}
if (/\.([cm]?ts|tsx)$/.test(filename))
return {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge the object with previous one.

Copy link
Contributor Author

@tmkx tmkx Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how to merge the objects? it's a union type:

type ParserConfig = TsParserConfig | EsParserConfig

src/plugins/cjs-interop.ts Outdated Show resolved Hide resolved
@sxzz
Copy link
Collaborator

sxzz commented Apr 29, 2024

Hmmm, can we just enable spiltting automatically if cjsInterop is enabled because of the problem #1056 (comment)? And add the notice to the docs

@tmkx
Copy link
Contributor Author

tmkx commented Apr 29, 2024

  1. splitting with cjs has edge cases: export issues with cjs #255
  2. it's also broken when a plugin attaches named exports
Input
// src/index.ts
const a = {
  hello: 'world',
};

export default a;
// tsup.config.ts
export default defineConfig({
  entry: ['./src/index.ts'],
  format: 'cjs',
  cjsInterop: true,
  splitting: true,
  plugins: [
    {
      name: 'add',
      renderChunk(code) {
        return {
          code: code + `\nconst newValue = 1;\nexport { newValue };`,
        };
      },
    },
  ],
});
Output
"use strict";Object.defineProperty(exports, "__esModule", {value: true});// src/index.ts
var a = {
  hello: "world"
};
var src_default = a;


exports.default = src_default;

const newValue = 1;
exports.newValue = newValue;
module.exports = exports.default;

@sxzz sxzz force-pushed the dev branch 2 times, most recently from 115a451 to 57654f6 Compare July 16, 2024 21:02
@sxzz sxzz deleted the branch egoist:main September 17, 2024 05:23
@sxzz sxzz closed this Sep 17, 2024
@sxzz sxzz reopened this Sep 17, 2024
@sxzz sxzz changed the base branch from dev to main September 17, 2024 05:39
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

Successfully merging this pull request may close these issues.

3 participants