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

Test creating functional components #477

Draft
wants to merge 16 commits into
base: master
from
Draft

Conversation

@Deraen
Copy link
Member

Deraen commented Feb 5, 2020

Current status: nearly all test cases are working with the new implementation, done some testing with real work projects and seems to be working.

Next steps:

  • Create snapshot release for testing
  • Find a way to compare performance to Class components
@Deraen Deraen force-pushed the feature/functional-components branch from f57b39a to 2d171ba Feb 5, 2020
@Deraen Deraen force-pushed the feature/functional-components branch 2 times, most recently from 1e75fbf to 574cebd Feb 28, 2020
@Deraen Deraen force-pushed the feature/functional-components branch from bfff84a to ea403d8 Mar 28, 2020
Render/as-element and other functions now take additional parameter for
setting options.

Currently only option is :functional-reag-elements?,
if set to true, by default Reagent components are
created as Functions, which behave similarly to Classes,
but allow using Hooks.
@Deraen Deraen force-pushed the feature/functional-components branch from dd8f422 to 94db25c Mar 28, 2020
@Deraen

This comment has been minimized.

Copy link
Member Author

Deraen commented Mar 28, 2020

Creating functional components is now optional, and can be enabled by providing options to render call:

(rdom/render [...] {:functional-reag-elements? true})

This will affect the tree of components created directly from given component. Using as-element or create-class will require providing the options again.

This is just first version of testing the idea of providing options to control Reagent, but it looks promising. I'm still unsure if having to provide options for as-element and such is a problem, and we could just encourage users to create their own namespace where they wrap the functions with their options.

Might be also possible to use Context or maybe even dynamic var, to set the default options for calls without options.

Another problem with this version is, that several of the caches need to be per options, but currently are global. This will probably be solved by creating Compiler type, where Compiler instance contains the state for caches, this compiler is created using the options and then passed to functions.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 28, 2020

Codecov Report

Merging #477 into master will increase coverage by 0.74%.
The diff coverage is 89.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #477      +/-   ##
==========================================
+ Coverage   82.90%   83.64%   +0.74%     
==========================================
  Files           9        9              
  Lines         737      795      +58     
  Branches      123      130       +7     
==========================================
+ Hits          611      665      +54     
- Misses         96       97       +1     
- Partials       30       33       +3     
Impacted Files Coverage Δ
src/reagent/ratom.cljs 93.26% <ø> (ø)
src/reagent/dom.cljs 85.00% <33.33%> (-4.48%) ⬇️
src/reagent/core.cljs 69.56% <50.00%> (+0.44%) ⬆️
src/reagent/impl/component.cljs 83.58% <89.79%> (+1.31%) ⬆️
src/reagent/impl/template.cljs 70.34% <93.54%> (+1.86%) ⬆️
src/reagent/dom/server.cljs 100.00% <100.00%> (ø)
src/reagent/impl/util.cljs 97.40% <100.00%> (+0.47%) ⬆️

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 2e464e4...78b26e2. Read the comment docs.

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

Successfully merging this pull request may close these issues.

None yet

1 participant
You can’t perform that action at this time.