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

[BUGFIX] Contextual component variable transformation #230

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rajasegar
Copy link
Member

Fixes #220

I set out to solve the blockParams problem , now I have one more failing test case
@tylerturdenpants @Turbo87 Anything I am missing here?

package.json Outdated
@@ -22,6 +22,7 @@
"scripts": {
"coveralls": "cat ./coverage/lcov.info | node node_modules/.bin/coveralls",
"debug:integration": "node --inspect-brk ./test/run-test.js",
"debug:test": "node --inspect-brk ./node_modules/.bin/jest --runInBand --testNamePattern",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems unrelated to the PR?!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, sorry my bad, it was meant to be in a separate PR, will remove it.

) {
return transformNode(node, fileInfo, config);
}
},
BlockStatement(node) {
let tagName = `${node.path.original}`;
havingBlockParams = node.program.blockParams.length > 0;
if (havingBlockParams) {
currentBlockParams = node.program.blockParams;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't work for nested block params

Copy link
Member Author

Choose a reason for hiding this comment

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

@Turbo87 Can you give me an example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the corresponding template-lint rule has something like this implemented, or you can have a look at the implicit-this codemod

@tylerturdenpants
Copy link
Collaborator

@rajasegar I should have a branch pushed to origin for working on this issue. Let’s collaborate. My implementation keeps track of scope (yours might, but I have looked into it) and takes a more conservative approach to the transform when we can decide if a plain value is being yield or a contextual component.

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 12, 2020

FWIW with the new path param of the visitor this kind of tracking becomes a lot easier 😉

@tylerturdenpants
Copy link
Collaborator

tylerturdenpants commented Jan 12, 2020

@Turbo87 thats awesome. I haven’t had a chance to catch up. My current implementation was fairly straight forward, would like your thoughts on it anyway. Be that as it may might point was to make sure @rajasegar knew we could collaborate before he went any further or he can continue fixing it if he likes.

@sophiaonion
Copy link

quick question on this: Isn't it this supposed to be a feature? looking at this ember doc https://guides.emberjs.com/release/upgrading/current-edition/templates/
"Yielded components can also be invoked with angle bracket syntax".

The problem I see is that the codemod doesn't do this consistently

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.

Incorrect transformation of use of contextual component variable into a component invocation
4 participants