-
Notifications
You must be signed in to change notification settings - Fork 204
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 a stripExtensions option for RN support #205
Conversation
Codecov Report
|
i don't understand how this module works but y is : |
Actually you can think of it as this following list of steps :
With this PR :
|
@MatthieuLemoine thanks for the explanation, yeah makes it easy. so i guess what i'm saying is at this step: So the steps look like:
Notice the change on step 2. we loop through extensions array and remove every possible extension. since |
Yeah we could do that but it will be a breaking change.
That's why I chose to add an option so that it will be transparent for all the users that don't need this. |
ah i see. that makes sense. i guess a |
Yes I thought about that. It's more about flexibility. For now I encountered this issue only with React Native where the extensions we want to strip are the same that those we want to support. I don't know about all the use cases. But I'm willing to change the option name/type/value. |
Thank you for the contribution @MatthieuLemoine. When it's What do you think? cc @fatfisz |
@tleunen that's what i was getting at here: #205 (comment). always strip would be awesome if it's not a blocker. |
README.md
Outdated
@@ -82,6 +83,25 @@ To use the backslash character (`\`) just escape it like so: `'\\\\'` (double es | |||
|
|||
If you're using ESLint, you should use [eslint-plugin-import][eslint-plugin-import], and [eslint-import-resolver-babel-module][eslint-import-resolver-babel-module] to remove falsy unresolved modules. | |||
|
|||
## Usage with React Native | |||
|
|||
To let the packager resolve the right module for each platform, you have to add the ```.ios.js```and ```.android.js``` extensions and tell the plugin to strip these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One backtick is enough - also a space is missing before "and" ;)
I need to get into the details, but this looks good! Just one thought I already have - if we're putting the same extensions in two places, maybe something like |
I believe it's safe to strip it by default. I mean, to me it looks like it's a bug we're not doing that now. |
3920880
to
a04ecea
Compare
I've updated the PR to strip by default. All tests are still passing. |
Thanks @MatthieuLemoine, I'll check the branch in one of my project tonight to make sure everything still work as before for a basic project, and I'll merge and release a new beta after if all is green :) With this update, I believe we're really close to release the final version. |
See #90 for context.
Example :
We let the RN packager to choose which file to import depending on the platform.