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

Fix exports.js exporting unnamed functions #24

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@slorber

slorber commented Jan 9, 2017

Hi,

exports.thunks2 = function() {}; is currently exported as export function() {}; which is illegal

My PR is an attempt to fix this issue. I'm a total JSCodeShift/Recast beginner, but my PR pass the fixture test I created so it looks safe.

Also note that I upgraded to latest JSCodeShift and fixed non-passing unit-tests due to code-style preferences (expected import {module1,module2}, actual import { module1, module2 })

@slorber

This comment has been minimized.

slorber commented Jan 9, 2017

I've noticed another case I can handle:

exports.thunks2 = function someFnName() {}; should rather be transformed as export function thunks2() {}; instead of export function someFnName() {}; right?

Edit: commited a fix for that case: we always use the exports.propertyName even if it overrides an already defined function name

Another case that can be harder to fix would be to remplace usages of exports.propertyName in code, like I had in my codebase: export const ItemList = PT.arrayOf(exports.Item);

when exporting functions, the function name should be set to the expo…
…rts.propertyName, even if this overrides an existing function name (see #24 (comment))
@xjamundx

This comment has been minimized.

Contributor

xjamundx commented Jan 9, 2017

exports.thunks2 = function someFnName() {};

could maybe go to:

export let thunks2 = function someFnName() {}
@slorber

This comment has been minimized.

slorber commented Jan 10, 2017

hmmm sorry but I'm not sure to know how to do that with JSCodeshift yet :)

@gmathieu

This comment has been minimized.

Collaborator

gmathieu commented Mar 9, 2017

#31 implemented the fix

@gmathieu gmathieu closed this Mar 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment