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 support for class components #299

Merged
merged 3 commits into from
Nov 15, 2020

Conversation

Stijn98s
Copy link
Contributor

Closes #297

@Stijn98s Stijn98s marked this pull request as ready for review November 14, 2020 20:11
@Stijn98s
Copy link
Contributor Author

@lmiller1990 what do you think about this fix?

@lmiller1990 lmiller1990 self-requested a review November 15, 2020 08:02
Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Nice job, logic seems fine (bit dirty, but 🤷‍♂️ I don't see a cleaner way right now).

Can you address the one comment I left? Thanks!

tempOutput.match(/\}\(.*.?Vue\);/) &&
tempOutput.includes('vue-class-component')
) {
node.add(';exports.default = {...exports.default.__vccBase};')
Copy link
Member

Choose a reason for hiding this comment

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

I think we will now end up with 2x node.add(';exports.default = {...exports.default};') (added here and line 43).

Can you 1) check if that is the case and 2) make sure we only add it once? If we aren't careful, this can become really complex really fast!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

node now ends with

';exports.default = {...exports.default.__vccBase};',
';exports.default = {...exports.default, render};' 

or

';exports.default = {...exports.default.__vccBase};',
';exports.default = {...exports.default};' 

I think this is the desired behavoir because otherwise the code will be more complex.

Example:

if (
  tempOutput.match(/\}\(.*.?Vue\);/) &&
  tempOutput.includes('vue-class-component')
) {
if (tempOutput.includes('exports.render = render;')) {
    node.add(';exports.default = {...exports.default.__vccBase, render};')
  } else {
    node.add(';exports.default = {...exports.default.__vccBase};')
  }
} else {
  if (tempOutput.includes('exports.render = render;')) {
    node.add(';exports.default = {...exports.default, render};')
  } else {
    node.add(';exports.default = {...exports.default};')
  }
}

this will have an output of

';exports.default = {...exports.default.__vccBase, render};'

or

';exports.default = {...exports.default.__vccBase};'

Copy link
Member

Choose a reason for hiding this comment

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

Right, true, that is very complex. Fair enough.

I wonder if we should create a isClassComponent function 🤔

Anyway, this will do for now, I will test it out a bit and if it's all good do a release. thanks!

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

3 participants