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

fix: deprecate default export in favor of named export #641

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jun 14, 2024

This deprecates the default export in favor of the new named export sveltePreprocess. It's done to ensure a better interop between CJS and ESM without resorting to hacks in the future. It also enables people using "module": "NodeNext" in their tsconfig.json to import without type errors. The sub exports were also adjusted so that the transpiled TS output doesn't include __importDefault wrappers, which makes Node's static analysis miss those named exports.

Related: #591 (comment)

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to run pnpm lint!)

Tests

  • Run the tests with npm test or pnpm test

This deprecates the default export in favor of the new named export `sveltePreprocess`. It's done to ensure a better interop between CJS and ESM without resorting to hacks in the future. It also enables people using `"module": "NodeNext"` in their `tsconfig.json` to import without type errors.
The sub exports were also adjusted so that the transpiled TS output doesn't include `__importDefault` wrappers, which makes Node's static analysis miss those named exports.

// both for backwards compat with old svelte-preprocess versions
// (was only default export once, now is named export because of transpilation causing node not to detect the named exports of 'svelte-preprocess' otherwise)
export default babel;
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need the default export here? IIRC these were re-exported with names in the package entrypoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doing this out of fear that someone may have done import babel from 'svelte-preprocess/.../babel'. We should definitely remove it in the next major

Copy link
Member

Choose a reason for hiding this comment

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

I find that a tad unlikely, but that's an unfortunately good point 🥲

@dummdidumm dummdidumm merged commit a43de10 into main Jun 14, 2024
20 checks passed
@dummdidumm dummdidumm deleted the default-imports-are-stupid branch June 14, 2024 09:56
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.

None yet

2 participants