Skip to content

feat(wasm): Switch to TypeScript & named exports#363

Merged
NotWoods merged 1 commit intorollup:masterfrom
NotWoods:feat/wasm/types
May 19, 2020
Merged

feat(wasm): Switch to TypeScript & named exports#363
NotWoods merged 1 commit intorollup:masterfrom
NotWoods:feat/wasm/types

Conversation

@NotWoods
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 pushed 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
Copy Markdown
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.

2 participants