Fixes #4072 - Properly handle #require Member Expresions when perform…

[jonmbake]

Q	A
Bug fix?	yes
Breaking change?	no
New feature?	no
Deprecations?	no
Spec compliancy?	yes
Tests added/pass?	yes
Fixed tickets	#4072
License	MIT
Doc PR
Dependency Changes

The AMD transformer was not properly handling MemberExpressions with a #require object.

The AMD visitor previously always removed the require CallExpression path, which led to a "Property object of MemberExpression expected node to be of a type ["Expression"] but instead got null" error when a property was referenced on the #require call, for example require('foo').bar

The solution was to replace the object node in the MemberExpression with the source identifier when object expression is a valid require call.

Also, added unit tests for the various ways require is handled in the AMD transform.

…ing an AMD transform

[codecov-io]

Current coverage is 89.35% (diff: 100%)

No coverage report found for master at 53de56d.

Powered by Codecov. Last update 53de56d...e6c5f06

[jonmbake]

@hzoo Do you know of somebody that could work with me to get this merged? I am attempting to transpile Enzyme using the AMD transformer and this bug is a road block: enzymejs/enzyme#90. Thanks!

[loganfsmyth]

It seems like the confusion here may be the goal of this transforms. This transform is meant to convert ES6 module syntax into AMD, it is not trying to convert require calls. The only reason it handles require is because currently this gets run through ES6 modules => CommonJS modules => AMD modules. It only attempts to handle exactly the format output by the CommonJS transform, which means there should never be a MemberExpression require.

The problem you are having is because you are using require directly instead of using ES6 module syntax.

[jonmbake]

@loganfsmyth Thanks for the feedback. That makes sense.

The problem I have (and was attempting to fix with this PR) is that doing an AMD transform on: require('foo').bar should never blow up with that error. I think a better behavior would be to assume require('foo') is not a "valid require call" when the object of a Member Expression and just do a NO-OP transform. Would this be acceptable?

[jonmbake]

@loganfsmyth Also, RequireJS provides a CommonJS wrapper: http://requirejs.org/docs/commonjs.html#manualconversion... Might be a cleaner solution to just use that instead of doing all the business with removing the require node: https://github.com/jonmbake/babel-plugin-transform-es2015-modules-requirejs/blob/master/src/index.js. That is if RequireJS is the only supported AMD library.