Skip to content

Support for Functional Components#50

Merged
vire merged 20 commits intovire:masterfrom
candyapplecorn:master
Mar 13, 2018
Merged

Support for Functional Components#50
vire merged 20 commits intovire:masterfrom
candyapplecorn:master

Conversation

@candyapplecorn
Copy link
Copy Markdown
Contributor

@candyapplecorn candyapplecorn commented Mar 9, 2018

Please check if the PR fulfills these requirements

  • [x ] The commit message follows our guidelines: CONTRIBUTING.md
  • [ x] There are no linting errors and code is properly formatted (your run before push yarn lint)
  • [ x] Tests for the changes have been added (for bug fixes / features)
  • [ x] Docs have been added / updated (for bug fixes / features)

I wrote pretty descriptive comments so that should count as docs right?

What kind of change does this PR introduce? (check one with "x")

[ x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
functional components aren't supported; jest-vue-preprocessor would throw an error. This would break any tests that involved a functional component. So automated build systems would reject any builds that have tests for code that has functional components, even though the functional components work just fine for every other use (like rendering them on a webpage).

What is the new behavior?
functional components are semi-supported. Error 'no script available to transform' is no longer thrown. HOWEVER, the return value of Vue.compile does NOT have an $el.querySelector. This is a big problem as it makes it very hard to actually test functional components.

I wrote a test in my company's codebase for a functional component and it involved recursively delving through its childrens' vNode lists and piecing their text keys together... It's nasty.

The test I've added for this PR specifically expects there to NOT be a querySelector on $el. Hopefully in the future this test will fail, which means functional components will have gained more support.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

If a component is functional and does not have a script tag, do not try to parse the non-existent script tag.
doesn't seem to be doing its job
so have to console.log json and then copy to local for testing...
don't edit in vim editor its bad mmkay
gimme back the last 4 minutes of my life
IT WAS AN OBJECT NOT A STRING
cuz the package I forked from seems to like that kinda style ;)
@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 9, 2018

Codecov Report

Merging #50 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #50   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines           4      4           
=====================================
  Hits            4      4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1df50d7...3b09971. Read the comment docs.

@vire
Copy link
Copy Markdown
Owner

vire commented Mar 9, 2018

@candyapplecorn thank you for your contrib, I see it's a small change, but could you introduce a test pls for this particular code you've added?

Otherwise LGTM

@candyapplecorn
Copy link
Copy Markdown
Contributor Author

candyapplecorn commented Mar 10, 2018

Sure, I will add a test today.
just found out what LGTM means Neat, still will add a test though.

joseph burger added 3 commits March 10, 2018 12:38
removed newlines

no breaking changes
a test documenting Vue's inability to cope with functional
components. throws a warning every time Vue tries to mount
one. $el is not defined.

breaking changes: none, but in the future $el should be
defined, as it can't be used for testing if it's
undefined
@candyapplecorn
Copy link
Copy Markdown
Contributor Author

Well, wow, what do you know. Stuff behaves differently on my local machine than on travis-ci.
This puts me in a conundrum.
Either I expect $el.querySelector not to be defined and my tests pass locally, or, I expect $el.querySelector to be defined and pass tests on codeship.
screen shot 2018-03-10 at 4 03 05 pm

joseph burger added 6 commits March 10, 2018 16:07
test behaves differently on travis-ci than
on my local machine, so now there is a test
to document that

breaking changes none
tried to use typeof, decided to use
casting

no breaking changes
context is needed for mounting functional components
but not as an option for a render function

no breaking changes
was passing an expression not a function
to onClick handler

no breaking changes
1. need to pass props in context object to Vue.compile
2. need '@click="props.onClick('stuff')" VS
	'@click="() => props.onClick('stuff')"

No breaking changes
can't reproduce $el.querySelector being defined
locally, yet on travis-ci it's defined.
Without it being defined I can't debugger and
figure out what the mock function isn't being called.

Breaking changes: none; objeys all test criteria
except for testing that mockFn was called
@vire vire merged commit c0c0590 into vire:master Mar 13, 2018
vire added a commit that referenced this pull request Mar 13, 2018
@vire
Copy link
Copy Markdown
Owner

vire commented Mar 13, 2018

Sry for prematurely merging this PR, I haven't tried out on my local machine. I assumed that you're only trying to check if the component is a functional component, but there's something more going on...

  1. pls don't use if-else without braces, it can produce side-effects and headache with debugging
    image

  2. with functional components the test was failing because you were accessing props which is kinda undefined in the template or I don't exactly know the API (haven't checked)

I'll try look into this again in couple of days

vire added a commit that referenced this pull request Mar 13, 2018
* Revert "chore(jest): Remove `mapCoverage` from jest settings (#56)"

This reverts commit 06008e1.

* Revert "ci(travis): Remove yarn installation on travis (#54)"

This reverts commit df9d528.

* Revert "chore(node): Update node engines in package.json to 8.9 (#53)"

This reverts commit 75d5f37.

* Revert "Support for Functional Components (#50)"

This reverts commit c0c0590.
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.

3 participants