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

Add options argument to register functions to pass to pirates #573

Merged
merged 1 commit into from Dec 30, 2020

Conversation

gordonmleigh
Copy link
Contributor

Further to #571, this PR exposes the new addHook options argument to the register* functions too.

Would you consider making the register functions available via the index, so they can be imported without a deep package reference (currently 'sucrase/dist/register')? I can add to this PR or another. If we do this I'll also update the readme to document these functions.

Thanks for the quick merge on the other PR!

@codecov-io
Copy link

Codecov Report

Merging #573 (d2b6f9f) into master (5795a83) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #573   +/-   ##
=======================================
  Coverage   81.93%   81.93%           
=======================================
  Files          55       55           
  Lines        5800     5800           
  Branches     1311     1311           
=======================================
  Hits         4752     4752           
  Misses        764      764           
  Partials      284      284           
Impacted Files Coverage Δ
src/register.ts 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5795a83...d2b6f9f. Read the comment docs.

@alangpierce
Copy link
Owner

Would you consider making the register functions available via the index, so they can be imported without a deep package reference (currently 'sucrase/dist/register')? I can add to this PR or another. If we do this I'll also update the readme to document these functions.

Unfortunately I think this would break Sucrase in the browser since it would mean a regular Sucrase import would pull in pirates, which seems to have a hard dependency on a node environment. I tested it with these steps and saw a webpack failure:

yarn build
yarn link
cd website
yarn link sucrase
yarn start
...
Module not found: Error: Can't resolve 'module' in '/Users/alanpierce/code/sucrase/node_modules/pirates/lib'

(The package named "module" is built into node, and pirates expects it to exist.)

I might be missing an alternate approach, though; happy to hear suggestions. Another high-level option would be to refactor out @sucrase/register and @sucrase/cli packages, though IMO it's nice for the basic things to be "batteries included".

Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

Thanks! As I mentioned, I'm unsure about the best way to export these functions in a cleaner way, so I'll merge this step and happy to discuss the other stuff separately.

@alangpierce alangpierce merged commit c93fcb2 into alangpierce:master Dec 30, 2020
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