search

Shopify / sprockets-commoner

Use Babel in Sprockets to compile JavaScript modules for the browser
MIT License
182 stars 22 forks source link

Not working in Node 6.2 #24

Closed lemonmade closed 8 years ago

lemonmade commented 8 years ago

I'm not sure exactly why this is breaking on newer Node, but an example app with the following index file fails to compile with Node 6.2.0:

import {map} from 'lodash';
console.log(map([1, 2, 3], (val) => val * 3);

Fails with the following error (as an aside, I had to dive into the source code to expose the error stack, might be worth exposing that here: https://github.com/Shopify/schmooze/blob/master/lib/schmooze/processor_generator.rb#L18):

TypeError: /Users/chris/Projects/illustrations/app/assets/javascripts/application.js: Path must be a string. Received undefined
    at assertPath (path.js:7:11)
    at Object.dirname (path.js:1324:5)
    at resolve.sync (/Users/chris/.gem/ruby/2.3.1/gems/sprockets-commoner-0.2.6/js/babel-plugin-sprockets-commoner-internal/node_modules/browser-resolve/index.js:281:21)
    at resolveTarget (/Users/chris/.gem/ruby/2.3.1/gems/sprockets-commoner-0.2.6/js/babel-plugin-sprockets-commoner-internal/index.js:124:26)
    at PluginPass.VariableDeclarator (/Users/chris/.gem/ruby/2.3.1/gems/sprockets-commoner-0.2.6/js/babel-plugin-sprockets-commoner-internal/index.js:166:18)
    at NodePath._call (/Users/chris/Projects/illustrations/node_modules/babel-traverse/lib/path/context.js:78:18)
    at NodePath.call (/Users/chris/Projects/illustrations/node_modules/babel-traverse/lib/path/context.js:49:17)
    at NodePath.visit (/Users/chris/Projects/illustrations/node_modules/babel-traverse/lib/path/context.js:108:12)
    at TraversalContext.visitQueue (/Users/chris/Projects/illustrations/node_modules/babel-traverse/lib/context.js:174:16)
    at TraversalContext.visitMultiple (/Users/chris/Projects/illustrations/node_modules/babel-traverse/lib/context.js:119:17)
    at TraversalContext.visit (/Users/chris/Projects/illustrations/node_modules/babel-traverse/lib/context.js:217:19)

I can fix this by changing https://github.com/Shopify/sprockets-commoner/blob/master/js/babel-plugin-sprockets-commoner-internal/index.js#L124 to read:

var resolvedPath = resolve(path, Object.assign({}, opts, {filename: file.opts.filename}));

This issue goes away in 5.7.0.

cc/ @Bouk @dfmcphee

bouk commented 8 years ago

Hmm seems browser-resolve does a weird thing where it always calls dirname on filename, even if it doesn't use it and goes with basedir instead (look at line 231 in browser-resolve/index.js). I'll PR to browser-resolve

bouk commented 8 years ago

Oh actually it's fixed already upstream https://github.com/defunctzombie/node-browser-resolve/pull/80

bouk commented 8 years ago

Version 0.2.7 has been released with this issue fixed