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

Add no-reassign-params #85

Merged
merged 1 commit into from
Feb 23, 2018
Merged

Conversation

elicwhite
Copy link
Collaborator

Adding this codemod because someone from open source might be interested in fixing the shadowing issue that keeps us from using this at Facebook.

A test for the issue we have at Facebook is commented out in the test file:

  // Shadowing function argument
  // This doesn't work yet and is the major blocker to us being able to
  // use this at Facebook.
  defineInlineTest(
    transform,
    undefined,
    `
function foo(x) {
  x = 2;
  var x = 4;
  return x;
}`,
    `
function foo({x}) {
  let localX = x;
  localX = 2;

  var localX2 = 4;
  return localX2;
}`,
  );

Reasonable to merge this in the mean time for others to get value. Most people don't run into this edge case.

@klvs
Copy link

klvs commented Feb 25, 2018

Full disclosure: I'm not codemod expert. Infact, I've never used jscodeshift so my solution might naive or plain wrong. It seems like interesting tech so I figured I'd give it a shot.

Anyway, I roughly have this working but am wondering if the object destructuring in the param for the test is intentional. i.e: function foo({x}) { should be function foo(x) {, no? Otherwise the codemod would be changing the signature which would be bad, right?

If not I'm probably misunderstanding something. If so, then I nearly have this working -- just need to clean it up and fix a test case I broke.

@elicwhite
Copy link
Collaborator Author

Whoops. Yep! The result should be function foo(x) {. Good catch. Sorry for adding some confusion to your first foray into codemods. :)

@elicwhite elicwhite deleted the no-reassign-params branch February 25, 2018 08:11
@elicwhite
Copy link
Collaborator Author

@klvs, I'm really excited for your fixes. I actually keep checking my github notifications hoping to see something from you. That is how excited I am. :P

Let me know if you have any questions that I can help you resolve.

@klvs
Copy link

klvs commented Feb 28, 2018

Oh okay. I'll open up a WIP soon. Took a little break but I'll send a PR your way in the next day or two. Thanks for the support.

@elicwhite
Copy link
Collaborator Author

@klvs Anything I can do to help you be successful with this? :)

@klvs
Copy link

klvs commented Mar 19, 2018

Not particularly. My classes caught up with me and I plan on finishing this (if it's not done) when they're done. I can put up a WIP PR if you'd like but the code is currently kinda hacky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants