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 callback and array prop detection in functional template #23

Merged
merged 7 commits into from
Nov 5, 2017

Conversation

agualis
Copy link
Contributor

@agualis agualis commented Nov 5, 2017

I should have reviewed the compiler books from my college times ;)

Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

I know this was from the previous PR, but can you change the isFunctionalTempalte check to this:

 const isFunctional = parts.template && && parts.template.attrs && parts.template.attrs.functional

@@ -1,11 +1,16 @@
module.exports = function extractProps (content) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename this file to extract-props, so that it matches the named exported function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

lib/process.js Outdated
@@ -34,7 +34,7 @@ function changePartsIfFunctional (parts) {
if (isFunctionalTemplate(parts)) {
parts.lang = 'javascript'
const functionalProps = extractPropsFromFunctionalTemplate(parts.template.content)
parts.template.content = parts.template.content.replace('props.', '')
parts.template.content = parts.template.content.replace(/props./g, '')
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you remove props from the template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this code is a bit cryptic. What I basically do is detecting all prop definitions through a regular expression and then I remove the "props." prefix cause it won't be present when parent templates pass properties (you can see how this works in process-functional.spec.js that will be renamed to extract-props.spec.js)

@eddyerburgh
Copy link
Member

I'll be honest, I wasn't sure what was happening in the last PR. I haven't looked into functional templates, but I saw you had a passing test so assumed it was working.

Can you tell me a bit more about why you need to extract props specifically? From this page — https://vue-loader.vuejs.org/en/features/functional.html — it looks like you can access anything on the context object, which contain a lot of properties like data and childrenhttps://vuejs.org/v2/guide/render-function.html#Functional-Components.

@agualis
Copy link
Contributor Author

agualis commented Nov 5, 2017

If I understood it correctly, is vue-loader the one that parses the SFC functional templates, extracts the inline prop definitions and passes the result to Vue.render in a webpack context.
But in vue-jest we are precisely avoiding the use of webpack.

I didn't want to import vue-loader code to make the everything lighter and faster.That's why I decided to do the parsing by hand. But it's not a trivial case and that's why the pull-request did not work straight forward with all possible cases.

Tell me if I'm losing something and I'll be happy to solve the problem in another way.

Thanks a lot for your amazing job BTW!

@eddyerburgh
Copy link
Member

Ah, so in that case the context refers to the functional render context object. Functional components receive a context object, because they are just functions, which means they can't access props using the this keyword.

I agree that we shouldn't port things over from vue-loader. It's fine to do the parsing by hand.

And thanks for the PR and for your work 😀

@eddyerburgh eddyerburgh merged commit 82f7a7e into vuejs:master Nov 5, 2017
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.

None yet

2 participants