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

Prevent double application of the transform #176

Merged
merged 4 commits into from
Jun 12, 2017

Conversation

fatfisz
Copy link
Contributor

@fatfisz fatfisz commented May 15, 2017

There were problems described e.g. in #167 wrt. the possible double application stemming from a not-exactly-good configuration. I think we should warn when this may happen and consider this an anti-pattern.

Example of such configuration:

alias: {
  'react-native-vector-icons': '@expo/vector-icons',
  '@expo/vector-icons/lib/create-icon-set': 'react-native-vector-icons/lib/create-icon-set',
  '@expo/vector-icons/lib/icon-button': 'react-native-vector-icons/lib/icon-button'
}

If there's a path react-native-vector-icons/lib/create-icon-set then first application of the plugin will transform it to @expo/vector-icons/lib/create-icon-set, and another application will result in react-native-vector-icons/lib/create-icon-set. This is actually causing trouble for users in some cases.


I feel the message could be greatly improved. Any suggestions?

Also this may deserve a section in the README describing the problem, which we could refer to in the message.

@fatfisz fatfisz requested a review from tleunen May 15, 2017 22:00
@codecov
Copy link

codecov bot commented May 15, 2017

Codecov Report

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

Impacted Files Coverage Δ
src/utils.js 100% <100%> (ø) ⬆️

@tleunen
Copy link
Owner

tleunen commented May 16, 2017

Maybe I need to read the thread again but I'm still having hard times understanding why someone would compile the code twice. Therefore I'm keeping this open for now, I'll read the thread again.

@fatfisz
Copy link
Contributor Author

fatfisz commented May 16, 2017

In some cases because of a misconfiguration the plugin may run twice (or possibly more) during one compilation. I'll try to come up with a minimal PoC later this day.

@fatfisz fatfisz force-pushed the warn-against-double-application branch from 3ec42bd to e9a567c Compare May 17, 2017 20:47
@fatfisz fatfisz mentioned this pull request Jun 4, 2017
@fatfisz fatfisz changed the title Add a warning for when double application is possible Prevent double application of the transform Jun 11, 2017
@fatfisz
Copy link
Contributor Author

fatfisz commented Jun 11, 2017

@tleunen I've updated this branch instead of making a new PR to resuse the test. I've used pathResolved instead of seen to reduce the possibility of a conflict with other plugins.

Only new tests added, the previous ones did not break (even with the CJS transform).

@tleunen
Copy link
Owner

tleunen commented Jun 12, 2017

Perfect, lets get this in. Thank you @fatfisz !

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

2 participants