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(wasm): Switch to TypeScript & named exports #363

Merged
merged 1 commit into from May 19, 2020

Conversation

NotWoods
Copy link
Member

@NotWoods NotWoods commented May 1, 2020

BREAKING CHANGES: Named exports are used for CJS

Rollup Plugin Name: wasm

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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.

plugins: [typescript()],
external: ['fs', 'path'],
output: [
{ format: 'cjs', file: pkg.main, exports: 'named' },
Copy link
Collaborator

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.

Copy link
Member Author

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?

Copy link
Collaborator

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 {
Copy link
Collaborator

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?

Copy link
Member Author

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.

@shellscape
Copy link
Collaborator

Gotta fix that pnpm lock and we should be good to merge

@shellscape shellscape closed this May 11, 2020
@shellscape shellscape reopened this May 11, 2020
@NotWoods NotWoods merged commit b899a82 into rollup:master May 19, 2020
@NotWoods NotWoods deleted the feat/wasm/types branch May 19, 2020 16:31
larrybotha added a commit to larrybotha/plugins that referenced this pull request May 20, 2020
# 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
@shellscape
Copy link
Collaborator

@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: ... to the merge commit messages before merging PRs that have them.

LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
@shellscape shellscape mentioned this pull request Jan 30, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants