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(sucrase): Pass disableESTransforms option and make types match options we support #984

Merged
merged 2 commits into from
Oct 1, 2021

Conversation

lydell
Copy link
Contributor

@lydell lydell commented Aug 28, 2021

Pull Request Requirements:

  • Please include tests to illustrate the problem this PR resolves.
  • Please lint your changes by running npm run lint before creating a PR. (I believe the correct command is pnpm lint, it failed though before I made changes)
  • Please update the documentation in /docs where necessary (Couldn’t find a “/docs” folder.)

Rollup Plugin Name: sucrase

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no
  • I’d expect TypeScript in this repo to be set up to catch misspellings. If you want a test you’d have to guide me how to do it.

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: none

Description

The disableESTransforms option was added in Sucrase 3.19.0.

The TypeScript types that ship with @rollup/plugin-sucrase says that disableESTransforms is a valid option, but supplying it makes no difference.

This passes the option on to Sucrase.

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
The `disableESTransforms` option was added in [Sucrase 3.19.0].

The TypeScript types that ship with `@rollup/plugin-sucrase` says that
`disableESTransforms` is a valid option, but supplying makes no
difference.

This passes the option on to Sucrase.

[Sucrase 3.19.0]: https://github.com/alangpierce/sucrase/blob/c15268c2cd1ef4d613f41978478a8061c483aaae/CHANGELOG.md#3190-2021-06-23
@lydell lydell requested a review from shellscape as a code owner August 28, 2021 16:26
@lydell
Copy link
Contributor Author

lydell commented Sep 26, 2021

@tjenkinson Friendly ping? (I saw you merged another PR a couple of days, I hope it’s ok to ping you! 😊 )

@tjenkinson
Copy link
Member

Looks good to me but I think it would be better if instead of the types for this package just omitting things from the sucrase types that we don’t support, it explicitly includes the types for the options we pass though like an allow list. Do you fancy having a go at that?

If we had that already this wouldn’t be a bug, it would be a feature that adds the new type and passes though the option.

@tjenkinson tjenkinson self-requested a review September 26, 2021 13:16
@lydell
Copy link
Contributor Author

lydell commented Sep 26, 2021

Do you fancy having a go at that?

Sorry, I don’t feel like I have time I can spend on that.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…ough
@tjenkinson
Copy link
Member

No problem I just pushed a commit to do it

Copy link
Member

@tjenkinson tjenkinson left a comment

Choose a reason for hiding this comment

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

LGTM!

@tjenkinson tjenkinson changed the title fix(sucrase): Pass disableESTransforms option fix(sucrase): Pass disableESTransforms option and make types match options we support Sep 26, 2021
@tjenkinson
Copy link
Member

Given I've pushed a change I probably shouldn't merge it. Pinging @shellscape

@shellscape shellscape merged commit b0f2e36 into rollup:master Oct 1, 2021
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

3 participants