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

Account for SequenceExpression in the utils #101

Merged
merged 1 commit into from
Oct 7, 2020
Merged

Account for SequenceExpression in the utils #101

merged 1 commit into from
Oct 7, 2020

Conversation

rrhg
Copy link

@rrhg rrhg commented Aug 27, 2020

Fix for #95.
The prop value with an expression type of SequenceExpression could not be resolved.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Let's add a test for this please :-)

*/
export default function extractValueFromSequenceExpression(value) {
// eslint-disable-next-line global-require
const getValue = require('./index.js').default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const getValue = require('./index.js').default;
const getValue = require('./').default;

(i realize there's other files that include the redundant "index.js" but let's not continue the mistake)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yarn test gave me this error:
" 11:28 error Useless path segments for "./", should be "." import/no-useless-path-segments"

So ended up with: const getValue = require('.').default;

@rrhg
Copy link
Author

rrhg commented Aug 27, 2020

Thanks !!!
Could you provide some guidance. I'm not sure because I can't find how other "expression extractor functions" in src/values/expressions/ are been tested.

I don't have much experience on this :) . What comes to mind is creating a new test file just for testing extractValueFromSequenceExpression() & pass a SequenceExpression object.

For example, playing with reactjs/react.dev#3120, I found out that, when running yarn lint in the reactjs.org repo, the object been passed to extractValueFromSequenceExpression(object) is :

{"type":"SequenceExpression","start":821,"end":842,"loc":{"start":{"line":32,"column":12},"end":{"line":32,"column":33}},"range":[821,842],"expressions":[{"type":"Literal","start":821,"end":827,"loc":{"start":{"line":32,"column":12},"end":{"line":32,"column":18}},"range":[821,827],"value":"alt-","raw":"'alt-'","_babelType":"Literal"},{"type":"MemberExpression","start":829,"end":842,"loc":{"start":{"line":32,"column":20},"end":{"line":32,"column":33}},"range":[829,842],"object":{"type":"Identifier","start":829,"end":837,"loc":{"start":{"line":32,"column":20},"end":{"line":32,"column":28},"identifierName":"language"},"range":[829,837],"name":"language","_babelType":"Identifier"},"property":{"type":"Identifier","start":838,"end":842,"loc":{"start":{"line":32,"column":29},"end":{"line":32,"column":33},"identifierName":"code"},"range":[838,842],"name":"code","_babelType":"Identifier"},"computed":false,"_babelType":"MemberExpression"}],"extra":{"parenthesized":true,"parenStart":820},"_babelType":"SequenceExpression"}

@ljharb
Copy link
Member

ljharb commented Aug 27, 2020

Here's a test for OptionalCallExpression:

it('should evaluate to a correct representation of optional call expression', () => {
const runTest = () => {
const prop = extractProp('<div foo={bar.baz?.(quux)} />');
const expected = 'bar.baz?.(quux)';
const actual = getPropValue(prop);
assert.equal(expected, actual);
};
if (fallbackToBabylon) {
// eslint-disable-next-line no-undef
expect(runTest).toThrow();
} else {
runTest();
}
});

@rrhg
Copy link
Author

rrhg commented Aug 27, 2020

The test is failing & I can't figure out why. But it turns out the test you used as example above, was also already failing with this msg:

  ● getPropValue › Member expression › should evaluate to a correct representation of optional call expression

    assert.equal(received, expected) or assert(received) 
    
    Expected value to be (operator: ==):
      null
    Received:
      "bar.baz?.(quux)"

To make sure it was already failing before I touched it, I cloned your repo again & run yarn & yarn test, and sure enough, it is failing. So I'm confused about what to do next.

@rrhg
Copy link
Author

rrhg commented Aug 27, 2020

I deleted previous test & created a new one in tests/src/getPropValue-flowparser-test.js and now it passed.

The previous test was failing because of the babel parser. So I think now is in the right place.

Keep in mind that other 2 tests are failing, but those were failing before I touched the project.

@rrhg
Copy link
Author

rrhg commented Sep 5, 2020

My apologies. Didn't realize had to do this small suggested change to the import.

Please let me know if there's anything else I should do. Thanks

@rrhg rrhg requested a review from ljharb September 5, 2020 16:19
@ljharb
Copy link
Member

ljharb commented Sep 5, 2020

looks like tests are failing :-/

@rrhg
Copy link
Author

rrhg commented Sep 5, 2020

Yes. As I explained in previous comment, those test were failing before I touched to repo. Maybe there's something I'm missing.

To make sure it was already failing before I touched it, I cloned your repo again & run yarn & yarn test, and sure enough, it is failing. So I'm confused about what to do next.

The tests failing are for file getPropValue-babelparser-test.js.
I initially put my test there but then deleted it & now my test is in file getPropValue-flowparser-test.js. This file is not failing the test.

@ljharb
Copy link
Member

ljharb commented Sep 5, 2020

ahh ok gotcha, i'll see if i can fix that up.

@rrhg
Copy link
Author

rrhg commented Sep 6, 2020

Do you think is a good idea to merge this one & then I can try to do some research about the other failing tests ?

I spent some time looking at the code & maybe found the problem with this test. The name implies a test for an expression of type MemberExpression with a nullable member, but is receiving type ChainExpression.

In src/values/expressions/index.js the expression object becomes null because the type ChainExpression doesn't exist. Maybe it could be fix by creating the ChainExpression account in utils & add it to all necessary functions.

I don't know the difference between MemberExpression & ChainExpression. My guest is that:

MemberExpression: <div foo={bar.baz} />
ChainExpression: <div foo={bar?.baz} />

Or maybe previously both were MemberExpression, & now they are divided ?. But you if guide me in the right direction, I can do it.
Thanks

@ljharb
Copy link
Member

ljharb commented Sep 6, 2020

Filed #102 to fix master.

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

Successfully merging this pull request may close these issues.

2 participants