Transforms broken when use ES7 Decorators or YAML

[markuplab]

Hello @substack. We use 2 transform (babel and yamlify). When we update to module-deps 4.0.5, browserify throw next error:

SyntaxError: Unexpected character '#'

This error in yaml file.

or detailed example with error stack:

Error: Parsing file /Users/k.kaysarov/src/account/_components/statistics/general/common/navigation/statistics-navigation.js: Unexpected character '@' (5:2)
    at Deps.parseDeps (/Users/k.kaysarov/src/account/node_modules/module-deps/index.js:452:28)
    at fromSource (/Users/k.kaysarov/src/account/node_modules/module-deps/index.js:389:44)
    at /Users/k.kaysarov/src/account/node_modules/module-deps/index.js:383:17
    at ConcatStream.<anonymous> (/Users/k.kaysarov/src/account/node_modules/concat-stream/index.js:36:43)
    at emitNone (events.js:72:20)
    at ConcatStream.emit (events.js:166:7)
    at finishMaybe (/Users/k.kaysarov/src/account/node_modules/readable-stream/lib/_stream_writable.js:511:14)
    at endWritable (/Users/k.kaysarov/src/account/node_modules/readable-stream/lib/_stream_writable.js:521:3)
    at ConcatStream.Writable.end (/Users/k.kaysarov/src/account/node_modules/readable-stream/lib/_stream_writable.js:486:5)
    at DuplexWrapper.onend (/Users/k.kaysarov/src/account/node_modules/readable-stream/lib/_stream_readable.js:545:10)

This error in js file

Error code example:

var { cut, extend } = require('catbee-utils');

class CampaignBudget {
  @extend({ css })
  render () {
    return this.$context.getWatcherData();
  }
}

module.exports = CampaignBudget;

If you need, i provide you example with code. Error reproduce only in 4.0.5, 4.0.4 work correctly.

[MellowMelon]

If it broke in 4.0.5, then it has to be this PR that broke it: https://github.com/substack/module-deps/commit/2f1a54d63846c0ada0debd16359025071c409108

Does your dependency hierarchy involve doing anything like requiring a file x inside of a node_modules directory and then requiring a file outside of any node_modules directory from x?

[markuplab]

Yes, i have some thing that require from node_modules. I use local package for easy require config file. It's require yaml files. And build system in catbee framework also require files from node_modules.

https://github.com/markuplab/catbee-boilerplate/tree/master/packages/config https://github.com/markuplab/catbee-boilerplate/blob/master/package.json#L14

[MellowMelon]

Requiring from node_modules is okay. The way 4.0.5 would deviate from past behavior is if something inside node_modules required something outside of node_modules.

What does the dependency graph look like between the entry point and the YAML + ES7 files that are failing?

[markuplab]

entry point require('config') // inside node_modules --> its require('./base.yml')

[markuplab]

https://github.com/markuplab/catbee-boilerplate - I don't test, but i think you can use it as example because it's based on our current project with yaml and es7.

Clone, npm i and node build

[MellowMelon]

Okay, that's what I figured. Indeed 4.0.5 would cause this to break.

Personally I feel like this usage pattern breaks the usual assumptions of modularity on node_modules, and it could also break on an npm dedupe. I'm inclined not to blame browserify for getting confused. But that might just be me being defensive about my own PR; I did consider this possibility and thought (incorrectly) that no one was actually doing this. I'll defer to @substack or other contributors, all more veteran than me, on whether this is a use-case we need to support.

[markuplab]

Ok, thanx for you help. It's critical problem for us, because our framework system based on browserify. Will be wait @substack response.

[markuplab]

@substack Can you join in this discussion? We still have problem.

[yoshuawuyts]

overloading require() is a bad idea, use brfs instead

[markuplab]

We don't overload require.

1. We have client side bundler based on browserify inside node_modules (i still don't understand why we can't use it outside codebase, and expose it as node module)
2. We use recommended by browserify wiki transfroms (yamlify for example).
3. We don't use any hacks, it's all basic operations from browserify api.

I think browserify must support internal builders, because it's very comfortable to use.

[yoshuawuyts]

require('./my-yaml') is not valid node code, therefore you're overloading it (using a browserify transform). This is problematic because (to my knowledge) AST transforms are applied before any other transforms, which requires require() to be validly resolved. If you were to use brfs, which is built specifically for this purpose, this would be a non-issue.

Personally I feel like this usage pattern breaks the usual assumptions of modularity on node_modules, and it could also break on an npm dedupe.

I support this statement; I'd recommend you use a different pattern - if you think that is not a viable option, then I cannot help you any further.

[markuplab]

It's problem repeated with babel. It's also overloading?) I think 4.0.5 version broke all ways to use ES6 transforms if your browserify instance located in node_modules. It's not minor release, i think we need 5.0.0 and 14.0.0 browserify with this fix, for libraries that use browserify instance on node_modules level.