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

Prevent getFunctionParent from returning Program #5923

Merged
merged 7 commits into from Aug 2, 2017

Conversation

@sarupbanskota
Contributor

sarupbanskota commented Jul 6, 2017

Works towards #5828

Q A
Patch: Bug Fix?
Major: Breaking Change? getFunctionParent will no longer return Program
Minor: New Feature?
Deprecations?
Spec Compliancy?
Tests Added/Pass?
Fixed Tickets #5828
License MIT
Doc PR
Dependency Changes
@jridgewell

We also need to remove the FunctionParent alias from the Program type definition.

@sarupbanskota

This comment has been minimized.

Show comment
Hide comment
@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

I expect lots of plugin tests to fail, but otherwise, to solve the original issue itself, do you think I got it right so far @jridgewell @boopathi?

Contributor

sarupbanskota commented Jul 13, 2017

I expect lots of plugin tests to fail, but otherwise, to solve the original issue itself, do you think I got it right so far @jridgewell @boopathi?

@@ -830,7 +832,7 @@ export default class Scope {
return scope;
}
} while ((scope = scope.parent));
throw new Error("We couldn't find a Function or Program...");
return null;

This comment has been minimized.

@boopathi

boopathi Jul 13, 2017

Member

why is this change required ?

@boopathi

boopathi Jul 13, 2017

Member

why is this change required ?

This comment has been minimized.

@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

because in another instance of the code, it freaks out #5923 (comment)

maybe there's a better way to deal with it though, open to suggestions :)

@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

because in another instance of the code, it freaks out #5923 (comment)

maybe there's a better way to deal with it though, open to suggestions :)

This comment has been minimized.

@boopathi

boopathi Jul 13, 2017

Member

Scope's getFunctionParent throws. paths' getFunctionParent doesn't throw and silently fails by returning null. Looks like you have swapped this behaviour.

@boopathi

boopathi Jul 13, 2017

Member

Scope's getFunctionParent throws. paths' getFunctionParent doesn't throw and silently fails by returning null. Looks like you have swapped this behaviour.

This comment has been minimized.

@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

to keep both consistent, what do you think of not throwing in both cases and returning null?
(although i'm letting scope's getProgramParent() throw since that would mean something is weird?)

@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

to keep both consistent, what do you think of not throwing in both cases and returning null?
(although i'm letting scope's getProgramParent() throw since that would mean something is weird?)

This comment has been minimized.

@loganfsmyth

loganfsmyth Jul 17, 2017

Member

I don't have strong feelings on this decision, but I do agree that the name and the behavior seem contradictory.

If we do go with this change, the doc comment on this function also needs to be updated.

@loganfsmyth

loganfsmyth Jul 17, 2017

Member

I don't have strong feelings on this decision, but I do agree that the name and the behavior seem contradictory.

If we do go with this change, the doc comment on this function also needs to be updated.

@@ -857,7 +857,7 @@ type BabelNodeExpressionWrapper = BabelNodeExpressionStatement | BabelNodeTypeCa
type BabelNodeFor = BabelNodeForInStatement | BabelNodeForStatement | BabelNodeForOfStatement;
type BabelNodeForXStatement = BabelNodeForInStatement | BabelNodeForOfStatement;
type BabelNodeFunction = BabelNodeFunctionDeclaration | BabelNodeFunctionExpression | BabelNodeObjectMethod | BabelNodeArrowFunctionExpression | BabelNodeClassMethod;
type BabelNodeFunctionParent = BabelNodeFunctionDeclaration | BabelNodeFunctionExpression | BabelNodeProgram | BabelNodeObjectMethod | BabelNodeArrowFunctionExpression | BabelNodeClassMethod;
type BabelNodeFunctionParent = BabelNodeFunctionDeclaration | BabelNodeFunctionExpression | BabelNodeObjectMethod | BabelNodeArrowFunctionExpression | BabelNodeClassMethod;

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 13, 2017

Member

So this is just an alias to BabelNodeFunction?

@nicolo-ribaudo

nicolo-ribaudo Jul 13, 2017

Member

So this is just an alias to BabelNodeFunction?

This comment has been minimized.

@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

yeah, I'm hoping that was the right thing to do ..

@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

yeah, I'm hoping that was the right thing to do ..

@@ -90,7 +90,9 @@ const collectorVisitor = {
//if (path.isFlow()) return;
// we've ran into a declaration!
path.scope.getFunctionParent().registerDeclaration(path);
const parent =
path.scope.getFunctionParent() || path.scope.getProgramParent();

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 13, 2017

Member

This wouldn't work, because getFunctionParent() either returns the function path or throws. It never returns a falsy value and thus getProgramParent() is never called.

@nicolo-ribaudo

nicolo-ribaudo Jul 13, 2017

Member

This wouldn't work, because getFunctionParent() either returns the function path or throws. It never returns a falsy value and thus getProgramParent() is never called.

This comment has been minimized.

@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

@nicolo-ribaudo you're correct, that's why I made it to return null if it can't find a function path - #5923 (comment)

@sarupbanskota

sarupbanskota Jul 13, 2017

Contributor

@nicolo-ribaudo you're correct, that's why I made it to return null if it can't find a function path - #5923 (comment)

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 13, 2017

Member

Oh sorry, I confused it with path.getFunctionParent() 😅

@nicolo-ribaudo

nicolo-ribaudo Jul 13, 2017

Member

Oh sorry, I confused it with path.getFunctionParent() 😅

@@ -33,7 +34,7 @@ export function find(callback): ?NodePath {
*/
export function getFunctionParent(): ?NodePath {
return this.findParent(path => path.isFunction() || path.isProgram());

This comment has been minimized.

@hzoo

hzoo Jul 18, 2017

Member

This should be documented as a breaking change too

@hzoo

hzoo Jul 18, 2017

Member

This should be documented as a breaking change too

@existentialism existentialism merged commit 75808a2 into babel:7.0 Aug 2, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 85.67% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sarupbanskota sarupbanskota deleted the sarupbanskota:enhancement/fix-getfuncparent-behavior branch Aug 3, 2017

wcjohnson added a commit to wcjohnson/lightscript-transform that referenced this pull request Sep 14, 2018

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