-
-
Notifications
You must be signed in to change notification settings - Fork 594
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(wasm): Switch to TypeScript & named exports #363
Conversation
packages/wasm/rollup.config.js
Outdated
plugins: [typescript()], | ||
external: ['fs', 'path'], | ||
output: [ | ||
{ format: 'cjs', file: pkg.main, exports: 'named' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the path forward that @lukastaegert laid out here: #360 (comment)
This is also an opportunity to create a "base" rollup config for plugins. Should help reduce duplication across the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we place a base config in root or next to the test utils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/shared/rollup.confg.js
would probably work at the root.
@@ -0,0 +1,14 @@ | |||
import { Plugin } from 'rollup'; | |||
|
|||
export interface RollupWasmOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this with the latest version of the typescript plugin, or will that do definition file output for us now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment the types folder is a bit cleaner. I'm still refining the declaration output for the typescript plugin.
Gotta fix that pnpm lock and we should be good to merge |
# By Tiger Oakes (2) and others # Via GitHub * master: feat(wasm): Switch to TypeScript & named exports (rollup#363) feat(node-resolve): Add default export (rollup#361) fix (sucrase): resolve directory imports (rollup#390) docs(typescript): update readme examples (rollup#391) # Conflicts: # packages/sucrase/test/snapshots/test.js.md # packages/sucrase/test/snapshots/test.js.snap # packages/sucrase/test/test.js # pnpm-lock.yaml
@NotWoods your commit message on merge didn't include the breaking changes, so the publish script won't pick up the breaking changes. If we update and force push that's going to cause trouble with pending PRs. Please make sure you're adding |
BREAKING CHANGES: Named exports are used for CJS
Rollup Plugin Name:
wasm
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: #360
Description
Switch to Typescript for the wasm plugin. I chose to also switch to named exports here, but that could be pulled out to a separate PR.