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

perf(navbar-toggle): convert template to render function #1313

Merged
merged 4 commits into from Nov 10, 2017

Conversation

Projects
None yet
3 participants
@tmorehouse
Member

tmorehouse commented Nov 8, 2017

Also removed no-longer used prop position

tmorehouse added some commits Nov 8, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Nov 8, 2017

Codecov Report

Merging #1313 into dev will decrease coverage by 0.04%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1313      +/-   ##
==========================================
- Coverage   42.69%   42.64%   -0.05%     
==========================================
  Files         130      130              
  Lines        2621     2619       -2     
  Branches      817      817              
==========================================
- Hits         1119     1117       -2     
  Misses       1189     1189              
  Partials      313      313
Impacted Files Coverage Δ
src/components/navbar/navbar-toggle.js 80% <80%> (ø)

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 acec621...2e0be12. Read the comment docs.

@tmorehouse tmorehouse changed the title from perf9navbar-toggle): convert template to render function to perf(navbar-toggle): convert template to render function Nov 8, 2017

@mosinve

This comment has been minimized.

Member

mosinve commented Nov 8, 2017

what reason to do this, is there any issue?
templates are converted to render functions at compile time by vue-compiler.
and, as for me, render functions impair the readability...

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 8, 2017

When compiling to the ES modules, the .vue template get copied over as is (no transpilation). and we have had reports of SSR bail rrors for users importing the es module version of Bootstrap-Vue (eitehr as a whole, or individual components). This is due to issues with vue-loader configurations for various users (in many cases, the imported .vue templates were rendering a single extra space during SSR and when hydrating on the client side Vue would bail because of the extra space.

Converting the .vue files to render functions (although not as legible as templates) avoids this SSR hydration bail.

The only other option is to have some vue-compiler generate the components as .js files in the es modules directory (instead of copying over the raw .vue files)

@mosinve

This comment has been minimized.

Member

mosinve commented Nov 8, 2017

@mosinve

This comment has been minimized.

Member

mosinve commented Nov 8, 2017

I think the issue not in vue-compiler, but in rollup-plugin-vue, as vue-loader uses vue-compiler and supports SSR, but i'm not sure Rollup is.
So, if we tend to use Rollup as main bundler, then our only option now - to avoid *.vue templates

@mosinve

mosinve approved these changes Nov 8, 2017

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 10, 2017

These would be similar to the render functions used in our functional components. Although functional components now support using single file components (using the latest vue-loader version).

The issue is not really the .vue templates, but the <template> section inside the .vue component file that gets mucked up by vue-loader when using the es modules.

@tmorehouse tmorehouse merged commit 88657fb into dev Nov 10, 2017

2 checks passed

License Compliance License checks passed.
Details
ci/circleci Your tests passed on CircleCI!
Details

@tmorehouse tmorehouse deleted the renderfn/navbar-toggle branch Nov 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment