-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Upgrade to Babel 7 and @babel scoped packages #255
Conversation
Hi, can this PR be merged? What about the unit test mentioned above? |
@vcarel This PR can be merged without any problems, but the tests will not cover error emission. |
Would it be difficult to keep the compatibility with Babel 6 and support Babel 7 through |
@julien-f Generally me and the Babel team have been recommending against using the bridge unless you really are in a place where you have to support multiple versions via one package, but using gulp-babel doesn't seem like a case where that is likely to be needed. |
Makes sense, thanks :) |
@ylemkimon Great PR but I can't seem to get your version to compile without throwing the Check this out: https://github.com/Keyframes/Keyframes.Pathfinder EDIT: Ok, this works now. Needed to set the --global flag on babelify as it can't require from node_modules. What a hot mess. |
Can a beta be published? |
I think we can replace class A {
// calling super() w/o a superclass
constructor () { super() }
} |
@krazyjakee Sorry for late response. You don't have to transpile the file( |
@goto-bus-stop Thank you for the idea! Actually, this revealed a bug which calls callback, even if transform has failed. |
I'm using ylemkimon's fork and it's working. |
Any plans to merge this soon? |
Not a huge fan of using a fork. Any chance this can be merged? |
Sorry for delay, yeah I wasn't watching this repo actually! We should update the readme too (doesn't have to be in this PR) |
index.js
Outdated
? Object.assign({sourceFileName: filename}, opts) | ||
: opts; | ||
var _opts = Object.assign({sourceFileName: sourceMapsAbsolute | ||
? filename : path.basename(filename)}, opts); |
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.
In the body, you say
Even without sourceFileName, Babel 7 maps source files to absolute path, if sourceMapsAbsolute is false(default), set sourceFileName to its base name
Could you clarify? Is this change attempting to fix an issue that was also present with Babel 6.x, or something that has regressed in 7.x? It feels like using the basename
here is likely to leave to conflicts where multiple files for instance have the name index.js
for instance.
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.
Sorry for the delay y'all 😓! Been busy with stuff so I appreciate everyone's work on this PR since it seems we had a lot of submissions as well.
Thank you so much for approving! I'm looking forward to using this. Can someone merge and release a beta? Much appreciated! |
We had a question in #255 (comment), but to land I'll just revert that. |
Ok releasing this as 9.0.0-beta.0 then. Seeing 2 test failures.. Just FYI but we're still a small group of volunteers and looking for more maintainers and help (I'm not really watching this repo anymore). If you'd like to help out it would be appreciated (via maintenance on babel projects or donating to our open collective or my patreon) |
Ok yeah need to determine if #255 (review) is a regression or not before release - makes more sense now. |
Could you release it as |
@hzoo I didn't know you had a patreon! Awesome. I just signed up :) |
These probably got copy pasted incorrectly in babel#255
Hm yeah, it looks like babel-core@6 defaulted |
This deals with a problem mentioned in [babel/babelify#255][0]. 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: ```js 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: ```js 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][1] to @babel/core's default options, so it's the same as back then. ```js 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. [0]: babel/babelify#255 [1]: https://github.com/babel/babel/blob/6689d2d23cdb607c326ed5a06855bfb84c050c56/packages/babel-core/src/transformation/file/index.js#L163-L172
This deals with a problem mentioned in [babel/babelify#255][0]. 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: ```js 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: ```js 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][1] to @babel/core's default options, so it's the same as back then. ```js 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. [0]: babel/babelify#255 [1]: https://github.com/babel/babel/blob/6689d2d23cdb607c326ed5a06855bfb84c050c56/packages/babel-core/src/transformation/file/index.js#L163-L172
* Fix default sourceFileName. This deals with a problem mentioned in [babel/babelify#255][0]. 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: ```js 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: ```js 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][1] to @babel/core's default options, so it's the same as back then. ```js 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. [0]: babel/babelify#255 [1]: https://github.com/babel/babel/blob/6689d2d23cdb607c326ed5a06855bfb84c050c56/packages/babel-core/src/transformation/file/index.js#L163-L172 * Use cwd-relative path for sourceFileName. * Revert sourceMap `file` property change. * fixup! Revert sourceMap `file` property change. * Fix whitespace change from merge conflict * Revert to using basename in source map outputs.
@hzoo Shouldn't this change be published to npm? Current workaround to use
|
@sqren please see #263, this is waiting on a new babel core release to address #255 (review) |
@goto-bus-stop Okay, thanks! |
These probably got copy pasted incorrectly in babel#255
These probably got copy pasted incorrectly in #255
require
to use @babel scoped packagesbabel.transform
babel.util.arrayify
,babel.util.canCompile
,result.metadata.usedHelpers
sourceFileName
, Babel 7 maps source files to absolute path, ifsourceMapsAbsolute
is false(default), setsourceFileName
to its base nameFixes #254, fixes #251, fixes #231