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 support for absolute path to babel plugin #229

Merged

Conversation

ChristopherBiscardi
Copy link
Contributor

When using Snowpack with a custom output directory like: snowpack --dest public/web_modules, the babel plugin can't use a path that looks like public/web_modules/import-map.json (like in this example) because you end up with ./web_modules/public/web_modules/import-map.json.

const path = require("path");
module.exports = {
  plugins: [
    [
      require.resolve("./forked-snowpack.js"),
      {
        importMap: "public/web_modules/import-map.json"
      }
    ]
  ]
};

This is because dir is automatically added to the path to require the import map in the plugin, even though the path to the import map isn't related to the web server web_modules path location at all (as far as I can tell).

This PR adds a backwards-compatible solution that lets people use an absolute path instead of a relative path. I think the better solution is to not use dir at all to construct the import path, but to remove that would be a breaking change so I opted for this solution, which is enough for my needs.

If this is useful and a good approach for y'all I can add test fixtures to the PR as well.

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

Makes sense! Would love an added test just to make sure that we don't regress in the future

assets/babel-plugin.js Outdated Show resolved Hide resolved
@ChristopherBiscardi
Copy link
Contributor Author

For the tests I had to add an additional call to babelPluginTester because the library doesn't support an options.js, only options.json, which is needed to get the absolute path to the import map.

@FredKSchott
Copy link
Owner

LGTM, fixtures solution makes sense, and we'll keep an eye on it going forward to make sure it doesn't get too messy

@FredKSchott FredKSchott merged commit de7f094 into FredKSchott:master Feb 27, 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.

2 participants