[fix] supply sourceRoot as default, solves symlink issues

[Swaagie]

This fixes babel/babel-loader#166 and babel/babel-loader#149. Afaik the sourceRoot is available in options and has correct absolute path.

[codecov-io]

Current coverage is 85.40%

Merging #3207 into master will decrease coverage by 2.29%

1. 8 files (not in diff) in ...ypes/lib/definitions were modified. more
   • Misses -4
   • Partials +13
   • Hits +8
2. 7 files (not in diff) in ...ages/babel-types/lib were modified. more
   • Misses -41
   • Partials +42
   • Hits -136
3. 3 files (not in diff) in ...l-traverse/lib/scope were modified. more
   • Misses +88
   • Partials +27
   • Hits +416
4. 3 files (not in diff) in ...raverse/lib/path/lib were modified. more
   • Misses -1
   • Partials +13
   • Hits +82
5. 3 files (not in diff) in ...e/lib/path/inference were modified. more
   • Misses -3
   • Partials +22
   • Hits -23
6. 11 files (not in diff) in ...el-traverse/lib/path were modified. more
   • Misses -19
   • Partials +82
   • Hits -7
7. 5 files (not in diff) in ...s/babel-traverse/lib were modified. more
   • Misses -13
   • Partials +19
   • Hits +53
8. 2 files (not in diff) in ...s/babel-register/lib were modified. more
   • Misses +1
   • Partials +12
   • Hits +1
9. 7 files (not in diff) in ...form-regenerator/lib were modified. more
   • Misses -17
   • Partials +33
   • Hits -29
10. 4 files (not in diff) in ...s2015-parameters/lib were modified. more
    • Misses -3
    • Partials +5
    • Hits +39

@@             master   #3207   diff @@
=======================================
  Files           193     197      +4   
  Lines          9385   12175   +2790   
  Methods        1068       0   -1068   
  Branches       2162    2504    +342   
=======================================
+ Hits           8230   10398   +2168   
- Misses         1155    1213     +58   
- Partials          0     564    +564   

Powered by Codecov. Last updated by 61b3a63...70874ea

[Swaagie]

What is the status on this PR?

[jmm]

@Swaagie Thank you for contributing! It needs to be reviewed, when people have the opportunity. Can you please provide a minimal reproduction that demonstrates what this is for without Webpack in the picture? If this were to move ahead there should be some new tests, correct?

Whoever else reviews this: I'd like the chance to review it before it gets merged if someone else doesn't decline it first.

[Swaagie]

I'll provide a minimal reproducible case through tests. That said I don't like pushing symlinks into repositories, but i'll check if I can get it to reproduce without symlinks. Also this isn't about WebPack. It just so happened that the bug was found there. This should be reproducible without webpack as well, any symlink outside the root folder would do...

[jardakotesovec]

I would love too see this merged.. just ran into this issue. I am using npm link to create symlink from node_modules to another package which I still need to get processed via babel and this PR make it work for me.

[jardakotesovec]

Actually I was able to reproduce it without symlink - just requiring file outside of the root folder.

require('../../folderOutside/component.js')

[jardakotesovec]

And seems that this fix does not cover situation when required file outside the root requires files which is back again inside root.

I know its bit unusual situation - we ran into it while some refactoring...

[Swaagie]

Sorry for the holdup, had a familiy addition 👶 to attend to last week ;) Working on it today

[jescalan]

Just want to pitch in that I'd love to see this fix. If there's anything I can help with would be happy to!

[Swaagie]

Finally managed to add a test against this option. Let me know if additional work is required

[Swaagie]

Tests failing due to Error: Cannot find module 'estraverse-fb', known issue, they work fine locally btw

[Swaagie]

What is the current status of this PR?

[jmm]

Hello @Swaagie. Thank you for providing additional information. I'm removing the awaiting reply label. The PR needs to be reviewed, when people have the opportunity. I personally want to take a close look at this. In my experience the only thing that makes sense to do by default with a sourceRoot value is to output it to the source map -- not use it in relation to input paths.

[Swaagie]

@jmm thanks for getting back to this, I'm fine if the solution is applied elsewhere. From my perspective this seemed like a logical point of entry. Using the local input path will ensure the symlink is not taken as source for discovery of the presets.

[loganfsmyth]

@Swaagie Usually we recommend people do:

    whateverBabelCompileFunction({
        presets: [
            require('babel-preset-es2015')
        ]
    })

any specific reason it's more desirable to do

    whateverBabelCompileFunction({
        sourceRoot: __dirname,
        presets: [
            'es2015'
        ]
    })

?

[Swaagie]

@loganfsmyth sure thats the current 'hotfix', I'm aware of that, however this is not really about directly providing sourceRoot. That option is set by default from babel-loader through webpack. Hence the DX becomes just providing es-2015 again in stead of resolving or providing the sourceRoot directly.

[Swaagie]

Is additional information required for this to land? Would happy to follow up or change whatever is needed

[jmm]

Hello @Swaagie. We'll keep you posted when additional information is required. Please understand that periods of inactivity generally mean that people haven't had the opportunity to review / discuss and respond -- there's no specific timeframe. But we are aware that there are open PRs. Babel doesn't have a policy requiring this, but in future situations I'd suggest considering opening an issue before a PR that very clearly explains the problem in the context of Babel, very preferably with a minimal reproduction that can be cloned and run and without relying on external dependencies like Webpack, and if you have an idea about a solution include that as well. Discussion about possible solutions can happen there before putting the effort into a PR.

This is one of several PRs that have been submitted, using several different approaches I believe, that are trying to fix an issue originating from babel-loader. I don't believe the approach in this PR is going to be successful (I'll elaborate a bit below), but if you still want it to be considered, please rework your branch as follows. Please don't include merge commits in a branch -- it makes it difficult to work with. If you need to get up to date with master, please rebase your branch on it, resolve any conflicts, and force push. Please include commit(s) with failing tests followed (respectively) by commit(s) that fix them.

It sounds like you're saying this approach relies on sourceRoot being set to an absolute local file system path by Webpack or babel-loader. If that's the case, that's relying on a certain correspondence of sourceRoot to local file system paths. But that relationship isn't inherent in the design of sourceRoot. Outputting it to the source map and using it that way is overloading it and in my experience conflicts with creating general purpose tooling. I believe that Babel should support the general case.

I think the approach in this PR will break stuff. For example, see the tests I added in #3196. If you add sourceRoot: "http://example.com/" here, it breaks the plugin / preset resolution.

[hzoo]

Closing for inactivity. Babel 7 resolving via cwd #5466 should fix symlink issues

[chpio]

Babel 7 resolving via cwd #5466 should fix symlink issues

hmm, include still gets resolved symlinks, so if i have a symlink in my node_modules of a package that needs to be transpiled by babel it's not enough to configure {include: ["node_modules/@my-pkgs/**/*"]} now i need to somehow figure out the "real" path of that package. which breaks pnpm, rush, lerna etc.