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 both default and named export #5

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

SukkaW
Copy link
Collaborator

@SukkaW SukkaW commented Jul 16, 2023

The default export breaks rollup -c rollup.config.ts --configPlugin swc3 --bundleConfigAsCjs: https://github.com/SukkaW/foxact/actions/runs/5567493053/jobs/10169409656

The PR adds both named export and default export.

@huozhi
Copy link
Owner

huozhi commented Jul 16, 2023

The change is legit but wonder why you have the issue, also saw you’re using esModuleInterop": true, in the tsconfig 🤔️

@SukkaW
Copy link
Collaborator Author

SukkaW commented Jul 17, 2023

The change is legit but wonder why you have the issue, also saw you’re using esModuleInterop": true, in the tsconfig 🤔️

I don't know, but I guess it is caused by how rollup bundles the rollup.config.ts. It looks like somehow rollup ignores the __esModule convention.

rollup-plugin-dts has the same issue as well: Swatinem/rollup-plugin-dts#247

@huozhi
Copy link
Owner

huozhi commented Jul 17, 2023

Ah looks like the official plugins are using a trick like https://github.com/rollup/plugins/blob/5ec2abe0325ed6b23bca26a5455d2a3b137e9e29/shared/rollup.config.mjs#L28

Which we don't have it, and --bundleConfigAsCjs makes it resolved as cjs. Does it work if you remove that flag?

src/index.ts Outdated
@@ -14,7 +14,7 @@ interface PreserveDirectiveMeta {
directives: Record<string, Set<string>>
}

export default function swcPreserveDirectivePlugin(): Plugin {
export function swcPreserveDirective(): Plugin {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
export function swcPreserveDirective(): Plugin {
function swcPreserveDirective(): Plugin {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in adee1ff (#5).

src/index.ts Outdated
@@ -103,3 +103,5 @@ export default function swcPreserveDirectivePlugin(): Plugin {
}
}
}

export default swcPreserveDirective
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can create a named export with a shorter name just other rollup plugins

Suggested change
export default swcPreserveDirective
export default swcPreserveDirective
export { swcPreserveDirective as preserveDirective }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in adee1ff (#5).

@huozhi huozhi changed the title chore: add both default and named export feat: add both default and named export Jul 17, 2023
Copy link
Owner

@huozhi huozhi left a comment

Choose a reason for hiding this comment

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

Thanks!

@huozhi huozhi merged commit f79dbf6 into huozhi:main Jul 17, 2023
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.

2 participants