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

Upgrade to Babel 7 and @babel scoped packages #255

Merged
merged 8 commits into from Apr 18, 2018
Merged

Conversation

ylemkimon
Copy link
Contributor

@ylemkimon ylemkimon commented Dec 30, 2017

  • Change dependencies/require to use @babel scoped packages
  • Use asynchronous babel.transform
  • Replace removed functions/properties: babel.util.arrayify, babel.util.canCompile, result.metadata.usedHelpers
  • Even without sourceFileName, Babel 7 maps source files to absolute path, if sourceMapsAbsolute is false(default), set sourceFileName to its base name
  • Updated test cases

Fixes #254, fixes #251, fixes #231

@ylemkimon ylemkimon changed the title Support Babel 7 and @babel scoped packages Upgrade to Babel 7 and @babel scoped packages Dec 30, 2017
@vcarel
Copy link

vcarel commented Jan 16, 2018

Hi, can this PR be merged? What about the unit test mentioned above?

@ylemkimon
Copy link
Contributor Author

@vcarel This PR can be merged without any problems, but the tests will not cover error emission.

@julien-f
Copy link

Would it be difficult to keep the compatibility with Babel 6 and support Babel 7 through babel-core@bridge?

@loganfsmyth
Copy link
Member

@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.

@julien-f
Copy link

Makes sense, thanks :)

@krazyjakee
Copy link

krazyjakee commented Jan 29, 2018

@ylemkimon Great PR but I can't seem to get your version to compile without throwing the ParseError: 'import' and 'export' may appear only with 'sourceType: module' error. There's no reason this shouldn't work while browserify has 7x version of @babel/preset-env set as a preset.

Check this out: https://github.com/Keyframes/Keyframes.Pathfinder
Clone and run npm install && npm run build

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.

@connorjclark
Copy link

connorjclark commented Feb 14, 2018

Can a beta be published?

@goto-bus-stop
Copy link
Contributor

I think we can replace babel-plugin-undeclared-variables-check in test/error.js with something else that throws an error. Maybe @babel/plugin-transform-classes with a snippet like this?

class A {
  // calling super() w/o a superclass
  constructor () { super() }
}

@ylemkimon
Copy link
Contributor Author

@krazyjakee Sorry for late response. You don't have to transpile the file(npm run es6). That's what babelify does do, and you can and must pass ES6 Javascript file browserify with babelify.

@ylemkimon
Copy link
Contributor Author

@goto-bus-stop Thank you for the idea! Actually, this revealed a bug which calls callback, even if transform has failed.

@ylemkimon
Copy link
Contributor Author

@vcarel @hoten Now it's ready to be merged or published.

@clubajax
Copy link

clubajax commented Mar 8, 2018

I'm using ylemkimon's fork and it's working.
"babelify": "github:ylemkimon/babelify"

@clarfonthey
Copy link

Any plans to merge this soon?

@chaines
Copy link

chaines commented Mar 30, 2018

Not a huge fan of using a fork. Any chance this can be merged?

@futagoza
Copy link

futagoza commented Apr 2, 2018

@hzoo Any plans to merge this?

@ylemkimon This should also close the previous related PR's: #241, #242, #244, #252

@hzoo hzoo requested a review from loganfsmyth April 2, 2018 14:27
@pioug pioug mentioned this pull request Apr 5, 2018
@hzoo
Copy link
Member

hzoo commented Apr 6, 2018

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);
Copy link
Member

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.

Copy link
Member

@hzoo hzoo left a 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.

@chrisgervang
Copy link

Thank you so much for approving! I'm looking forward to using this. Can someone merge and release a beta? Much appreciated!

@hzoo
Copy link
Member

hzoo commented Apr 18, 2018

We had a question in #255 (comment), but to land I'll just revert that.

@hzoo
Copy link
Member

hzoo commented Apr 18, 2018

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)

@hzoo hzoo merged commit c5e4d40 into babel:master Apr 18, 2018
@hzoo
Copy link
Member

hzoo commented Apr 18, 2018

Ok yeah need to determine if #255 (review) is a regression or not before release - makes more sense now.

@Blinnikov
Copy link

Could you release it as alpha if this unresolved question is so important?

@chrisgervang
Copy link

@hzoo I didn't know you had a patreon! Awesome. I just signed up :)

goto-bus-stop added a commit to goto-bus-stop/babelify that referenced this pull request Apr 20, 2018
These probably got copy pasted incorrectly in
babel#255
@goto-bus-stop
Copy link
Contributor

Hm yeah, it looks like babel-core@6 defaulted parserOpts.sourceFileName to basename(opts.filename), whereas @babel/core@7 defaults it to just opts.filename.

goto-bus-stop added a commit to goto-bus-stop/babel that referenced this pull request Apr 20, 2018
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
goto-bus-stop added a commit to goto-bus-stop/babel that referenced this pull request Apr 27, 2018
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
loganfsmyth pushed a commit to babel/babel that referenced this pull request Apr 27, 2018
* 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.
@sorenlouv
Copy link

sorenlouv commented May 5, 2018

@hzoo Shouldn't this change be published to npm?

Current workaround to use babelify with Babel 7:

"babelify": "git+https://git@github.com/babel/babelify.git#c5e4d4"

@goto-bus-stop
Copy link
Contributor

goto-bus-stop commented May 5, 2018

@sqren please see #263, this is waiting on a new babel core release to address #255 (review)

@sorenlouv
Copy link

@goto-bus-stop Okay, thanks!

goto-bus-stop added a commit to goto-bus-stop/babelify that referenced this pull request May 18, 2018
These probably got copy pasted incorrectly in
babel#255
loganfsmyth pushed a commit that referenced this pull request Sep 3, 2018
These probably got copy pasted incorrectly in
#255
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet