Fix default sourceFileName.

[goto-bus-stop]

Q	A
Fixed Issues?	babel/babelify#255 (review)
Patch: Bug Fix?	🙋‍♀️
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	🙋‍♀️
Documentation PR
Any Dependency Changes?
License	MIT

This deals with a problem mentioned in babel/babelify#255. I'm not
super sure about the implications, but it seems this may have been a
regression from Babel 6.

In babel@6, the default sourceFileName was the basename of the input
file:

require('babel-core').transform('var a = 10', {
  filename: __filename,
  sourceMaps: true
}).map
// { version: 3,
//   sources: [ 'index.js' ],
//   names: [ 'a' ],
//   mappings: 'AAAA,IAAIA,IAAI,EAAR',
//   file: 'index.js',
//   sourcesContent: [ 'var a = 10' ] } }

Currently however, the full file path is used:

require('@babel/core').transformSync('var a = 10', {
  filename: __filename,
  sourceMaps: true
}).map
// { version: 3,
//   sources: [ '/home/goto-bus-stop/Code/babel/repro-babelify-255/index.js' ],
//   names: [ 'a' ],
//   mappings: 'AAAA,IAAIA,IAAI,EAAR',
//   file: '/home/goto-bus-stop/Code/babel/repro-babelify-255/index.js',
//   sourcesContent: [ 'var a = 10' ] } }

This patch adds the path.basename() call that Babel 6 used to
@babel/core's default options, so it's the same as back then.

require('../babel/packages/babel-core').transform('var a = 10', {
  filename: __filename,
  sourceMaps: true
}).map
// { version: 3,
//   sources: [ 'index.js' ],
//   names: [ 'a' ],
//   mappings: 'AAAA,IAAIA,IAAI,EAAR',
//   sourcesContent: [ 'var a = 10' ] }

This is the desired behaviour for browserify at least, as it expects
relative paths in the source maps and rebases them to a root directory
when generating the final source map.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7784/

[loganfsmyth]

Taking the basename here is not right. The sourceFileName should include at least part of the file path.

Perhaps what we should do is change

filenameRelative = filename || "unknown",

to

filenameRelative = typeof filename === "string" ? path.relative(config.options.cwd, filename) || "unknown",

I'd also probably be in favor of removing unknown in favor of null but that could probably wait.

[hzoo]

Ok that sounds right, taking the actual relative path?

[loganfsmyth]

Yeah I think that'd be ideal. We didn't have cwd in 6.x so it wasn't as nice of an option. Once we land the config PR it might even make sense to use the root value, but cwd works for now.

[goto-bus-stop]

Gotcha, that makes sense. will check if browserify+babelify deals well with that too and update tonight.

[goto-bus-stop]

Updated to use cwd-relative paths, but it seems like it's not compatible with browserify's source map rebasing. I thought adding a file key to the source map might work but that doesn't appear to help. May need to set a sourceRoot in the source map or something instead

[loganfsmyth]

Looks like this needs to be removed since you reverted that change.

[loganfsmyth]

but it seems like it's not compatible with browserify's source map rebasing.

Are you familiar with how Browserify does that kind of thing? I'm not, but I'd be happy to try to figure it out.

What specifically are you seeing that's broken with the cwd-relative-path with Browserify?

[goto-bus-stop]

I'm somewhat familiar, but find the details hard to follow. The file paths I was seeing in the source map output after babelifying test/bundle/index.js with this patch were something like bundle/test/bundle/index.js.

Double checking now, it's actually a single test that's failing—when passing a different basedir into browserify. The basedir in this case is set to /path/to/babel/babelify/test, while the $CWD is /path/to/babel/babelify. So browserify identifies the file as bundle/index.js while babel writes test/bundle/index.js into the source map. (I'm not exactly sure why that makes the path rebasing work in this way, I would expect it to treat it as different roots.)

But making babelify pass browserify's basedir to babel as the cwd option fixes it! I think it would make sense to do that, since the basedir option is really exactly the same thing as babel's cwd option. babel/babelify#264 works for me with the changes in this PR.

[loganfsmyth]

Ugh, I'm sorry, I was wrong. I see now why it was using basename before. I didn't quite have the right mental model for how things were working.

You want the path of the filename in the .js.map to be relative to the location of the map itself. I was thinking of the map from the standpoint of the final output bundled file, but conceptually Babel's output is on a per-file basis, so it actually does make sense to use basename, and then leave it up to the bundler to rewrite the paths to be relative to the final bundle.

I still think the code in this PR is ideal, and the other PR you linked is sounds like a good idea to me too.

If you want to re-add that sourceFileName = path.basename(filenameRelative), line you had and then reverted, I'd be happy to land this.

[goto-bus-stop]

Gotcha! Updated. Babelify's sourcemap tests also pass without the other PR, if the basename is used. (I guess the basedir→cwd mapping still makes sense regardless.)