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

3.0.0-beta.0: causes HTML import plugin to use incorrect path #178

Closed
pglewis opened this issue May 16, 2017 · 13 comments
Closed

3.0.0-beta.0: causes HTML import plugin to use incorrect path #178

pglewis opened this issue May 16, 2017 · 13 comments
Assignees

Comments

@pglewis
Copy link

pglewis commented May 16, 2017

This works correctly for me in 2.7.0. I've cloned the poc repo with a commit that uses the babel-plugin-transform-html-import-to-string. The built file shows the results from running with v2.7.0. What little debugging I did, it appears as though beta may be causing the alias to be appended to the full path within the other plugin.

https://github.com/pglewis/babel-plugin-module-resolver-types-isimport-poc

Thanks for all the help thus far. I'm looking forward to implementing this because it will fix some annoying IDE configuration issues with our modules.

@fatfisz
Copy link
Contributor

fatfisz commented May 16, 2017

Thanks for the repo, I managed to reproduce the error.

@fatfisz fatfisz self-assigned this May 16, 2017
@fatfisz
Copy link
Contributor

fatfisz commented May 16, 2017

The reason for this is that the plugin now applies the transforms at the end of processing the file:

const visitor = {
  Program: {
    exit(programPath, state) {
      programPath.traverse(importVisitors, state);
    },
  },
};

Changing exit to enter fixes the issue.

@tleunen Would you have anything against changing this? I didn't feel strongly about this before, but now I came to the conclusion that this plugin should have a higher priority because there are plugins that depend on the paths being aliased.

The only plugins I could think of that could affect the code that module-resolver is working on are split into two categories:

  • plugins that change import to require - this doesn't affect module-resolver at all, because it works with both forms
  • plugins that substitute strings - it shouldn't matter if the string is in the import or the require call

So in the end I don't think this could break much stuff and would fix some other problems (like #84).

@fatfisz
Copy link
Contributor

fatfisz commented May 16, 2017

Just a note, changing exit to enter doesn't affect existing tests.

@tleunen
Copy link
Owner

tleunen commented May 16, 2017

Both plugins use exit. Do you mean that the html transform plugin ran before and thus causes an issue?

I thought the order of plugins in babel had an importance. So if module-resolver is set first, it should be executed before.

In the end, I don't mind changing our plugin to use enter. It's the right time to do it (before 3.0) if we want to change it. And indeed, it makes sense when other plugins depend on us, but again, I thought the order was important.

@fatfisz
Copy link
Contributor

fatfisz commented May 16, 2017

Here's a diagram that shows what's happening more accurately:

Program
 |
 |-- enter (no plugins here)
 |
 |-- Import
 |    |
 |    |-- enter (no plugins here)
 |    |
 |    \-- exit (transform-html-import-to-string kicks in - errors on invalid path)
 |
 \-- exit (module-resolver would fire up here and traverse the whole tree again)

@fatfisz
Copy link
Contributor

fatfisz commented May 16, 2017

So yes, the order of the plugins is preserved, it's just that the "exit" of module-resolver happens later than the "exit" of transform-html-import-to-string.

@tleunen
Copy link
Owner

tleunen commented May 16, 2017

Oh right, because the logic changed to exit on the program instead of require/import/export :)
Yep, I think we need to switch for enter then.

@fatfisz
Copy link
Contributor

fatfisz commented May 16, 2017

@pglewis This is fixed on master and should be released in a few hours on npm. Thanks for your report!

@tleunen
Copy link
Owner

tleunen commented May 16, 2017

3.0.0-beta.2 is out. But obviously I failed releasing it under the beta channel... Had an issue with beta.1 xD

@tleunen
Copy link
Owner

tleunen commented May 16, 2017

ok here we go. 3.0.0-beta.3 with the proper beta tag :)

But now beta.0 became the latest... wtf...

Roh people will understand it's a beta anyway :D

@pglewis
Copy link
Author

pglewis commented May 16, 2017

Thanks so much again. I'll test this myself later but I'm confident it'll be working for my setup.

@tleunen
Copy link
Owner

tleunen commented May 16, 2017

Thank you @pglewis :)

Let us know if you have any other issues with this beta.3 version

Hopefully we can soon release this 3.0

@pglewis
Copy link
Author

pglewis commented May 17, 2017

And for closure: everything works splendidly for my configuration with 3.0.0-beta.3. I've swapped out our old path aliasing plugin for this one and merged it. Life is good.

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

No branches or pull requests

3 participants