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

Does not work on ASTExplorer #102

Closed
pshrmn opened this issue Feb 8, 2019 · 1 comment · Fixed by #103
Closed

Does not work on ASTExplorer #102

pshrmn opened this issue Feb 8, 2019 · 1 comment · Fixed by #103

Comments

@pshrmn
Copy link
Contributor

pshrmn commented Feb 8, 2019

  • babel-plugin-macros version: 2.4.2+
  • node version: 11.9.0
  • npm (or yarn) version: 1.13.0

Relevant code or config

This commit (1785a0f) added a dependency of the resolve module, which internally uses fs.

const requirePath = resolve.sync(source, {

ASTExplorer uses Webpack's node setting to shim fs with an empty object.

https://github.com/fkling/astexplorer/blob/8b30e3d79498804bedfda4d4c802538fe0c60f5e/website/webpack.config.js#L240

What you did:

Attempted to use an import in the code (top left box) section on ASTExplorer.

What happened:

An i.statSync is not a function error

Reproduction repository:

n/a

Problem description:

  1. Go to ASTExplorer
  2. Activate the babel-plugin-macros transform
  3. Add an import statement in the code (top left) section.

Suggested solution:

path.relative?

@kentcdodds
Copy link
Owner

The reason path.relative won't work is because we need it to resolve like a require would. Can't use require.resolve because we need it to be relative to the file being compiled 😟

I'm open to any suggestions! We may be able to add an option to allow some to specify their own module resolution function. I think we've done sobering like that in the past for astexplorer.net. Anyone want to work on that?

kentcdodds pushed a commit that referenced this issue Feb 8, 2019
<!--
Thanks for your interest in the project. I appreciate bugs filed and PRs submitted!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

If you're new to contributing to open source projects, you might find this free
video course helpful: http://kcd.im/pull-request

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->

**What**:

Allows the plugin to accept a custom path resolve function.

<!-- Why are these changes necessary? -->

**Why**:

ASTExplorer doesn't have access to the `fs` module, so it throws an error when attempting to use `resolve.sync`.

<!-- How were these changes implemented? -->

**How**:

A `resolvePath` option is added to `macrosPlugin`, which is a function that receives the `source` path string and the `basedir` directory string. If `resolvePath` is not provided, `resolve.sync` will be used.

<!-- feel free to add additional comments -->

**Notes:**

I didn't see any tests for the existing `require` option for `macrosPlugin`, so it wasn't immediately obvious how I should test this. While not a substitute for a proper test, I did link it to a local clone of ASTExplorer and it worked as expected.

Fixes #102
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 a pull request may close this issue.

2 participants