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

Run plugin also on Program:exit #269

Merged
merged 2 commits into from Feb 8, 2018
Merged

Run plugin also on Program:exit #269

merged 2 commits into from Feb 8, 2018

Conversation

danez
Copy link
Contributor

@danez danez commented Feb 7, 2018

We have the usecase that one of our own transforms adds dynamic imports with paths which need to be replaced by this plugin.

This wasn't working because this plugin is run on Program:enter before all other plugins are executed.
I changed it so that the plugin now runs on both enter and exit to handle both of the usecases:

  • Other plugin wants to resolve/use path to module from import/require (needs replacement happen before -> Program:enter)
  • Other plugin inserts a new import/require (needs replacement happen afterwards -> Program:exit)

In order to avoid double transformation I added a list of already visited paths, so that each path only gets visited/transformed once.

@codecov
Copy link

codecov bot commented Feb 7, 2018

Codecov Report

Merging #269 into master will not change coverage.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/transformers/call.js 100% <100%> (ø) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/transformers/import.js 100% <100%> (ø) ⬆️

Copy link
Owner

@tleunen tleunen left a comment

Choose a reason for hiding this comment

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

This looks very good to me. Thank you @danez.

cc @fatfisz

@tleunen tleunen merged commit 12a2d07 into tleunen:master Feb 8, 2018
@fatfisz
Copy link
Contributor

fatfisz commented Feb 10, 2018

Hmm I'd be careful with this one - I vaguely remember a situation like this:

  1. module-resolver changed the path
  2. Another plugin replaced the node
  3. module-resolver changed the path again because there was no info about it being already resolved (a completely new node was there)

I don't think going back to this behavior is good and it might bite us soon.

@danez danez deleted the danez-patch-1 branch February 16, 2018 23:59
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