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 a stripExtensions option for RN support #205

Merged
merged 1 commit into from
Aug 17, 2017

Conversation

MatthieuLemoine
Copy link
Contributor

@MatthieuLemoine MatthieuLemoine commented Aug 9, 2017

See #90 for context.

Example :

#before.babelrc
{
  "presets": ["react-native"],
  "plugins": [
    [
      "module-resolver",
      {
        "root": ["./src"],
        "extensions": [".js", ".ios.js", ".android.js"]
      }
    ]
  ]
}

#after.babelrc
{
  "presets": ["react-native"],
  "plugins": [
    [
      "module-resolver",
      {
        "root": ["./src"],
        "extensions": [".js", ".ios.js", ".android.js"],
        "stripExtensions": [".ios.js", ".android.js"]
      }
    ]
  ]
}
/*
- module
  - index.ios.js
  - index.android.js
*/
// Before 
import module from 'module'; // -> import module from './module/index.ios';
// After
import module from 'module'; // -> import module from './module';

We let the RN packager to choose which file to import depending on the platform.

@MatthieuLemoine MatthieuLemoine changed the title ✨ Add a forceExtension option for RN support Add a forceExtension option for RN support Aug 9, 2017
@codecov
Copy link

codecov bot commented Aug 9, 2017

Codecov Report

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

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

@MatthieuLemoine MatthieuLemoine changed the title Add a forceExtension option for RN support Add a stripExtensions option for RN support Aug 9, 2017
@chirag04
Copy link

chirag04 commented Aug 9, 2017

i don't understand how this module works but y is : import module from 'module'; // -> import module from './module'; not the default behavior. y not always strip?

@MatthieuLemoine
Copy link
Contributor Author

MatthieuLemoine commented Aug 9, 2017

Actually you can think of it as this following list of steps :

  • convert to relative path : module -> ./module/index.ios.js
  • remove extension (using path.extname) : ./module/index.ios.js -> ./module/index.ios
  • remove index from path : ./module/index.ios -> ./module/index.ios (do nothing here because the filename is not index but index.ios)

With this PR :

  • convert to relative path : module -> ./module/index.ios.js
  • remove extension (using stripExtensions) : ./module/index.ios.js -> ./module/index
  • remove index from path : ./module/index -> ./module

@chirag04
Copy link

chirag04 commented Aug 9, 2017

@MatthieuLemoine thanks for the explanation, yeah makes it easy.

so i guess what i'm saying is at this step: remove extension (using path.extname) : ./module/index.ios.js -> ./module/index.ios
is user is providing a list of extensions then iterate through those extensions are remove them.

So the steps look like:

  • convert to relative path : module -> ./module/index.ios.js
  • remove extension (using extensions array or path.extname) : ./module/index.ios.js -> ./module/index
  • remove index from path : ./module/index -> ./module

Notice the change on step 2. we loop through extensions array and remove every possible extension. since ios.js is an extension we remove it from index.ios.js

@MatthieuLemoine
Copy link
Contributor Author

Yeah we could do that but it will be a breaking change.
@tleunen seemed to not want this : #90 (comment)

But at the same time, if the user sets a custom extension, the extension should be printed in the resulted path, in order for NodeJS or any other tooling system to pick it without issue.

That's why I chose to add an option so that it will be transparent for all the users that don't need this.

@chirag04
Copy link

chirag04 commented Aug 9, 2017

ah i see. that makes sense. i guess a stripExtensions: boolean could also work? not sure if there is a case where you want to strip some extensions and leave the rest.

@MatthieuLemoine
Copy link
Contributor Author

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.

@tleunen
Copy link
Owner

tleunen commented Aug 9, 2017

Thank you for the contribution @MatthieuLemoine.
After reading the description, I now understand better the problem. I wonder though if, instead of adding a new option, we should strip the extensions by default.

When it's .js, you don't want it since it's extra information not required. But if you specify the plugin to also work with .android.js and ios.js, these things actually become "extensions", so they should be stripped too.

What do you think?

cc @fatfisz

@chirag04
Copy link

chirag04 commented Aug 9, 2017

@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.
Copy link
Contributor

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" ;)

@fatfisz
Copy link
Contributor

fatfisz commented Aug 9, 2017

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 "extensions": [".js", "~.ios.js", "~.android.js"] could work instead?

@MatthieuLemoine
Copy link
Contributor Author

Well personally I'm fine with stripping the extension by default.

I can remove the option if you want @tleunen @fatfisz.

@tleunen
Copy link
Owner

tleunen commented Aug 11, 2017

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.
What do you think @fatfisz?

@MatthieuLemoine
Copy link
Contributor Author

I've updated the PR to strip by default. All tests are still passing.

@tleunen
Copy link
Owner

tleunen commented Aug 16, 2017

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.

@tleunen tleunen merged commit f22cb68 into tleunen:master Aug 17, 2017
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

4 participants