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

refactor(core): use hable #6271

Merged
merged 3 commits into from
Aug 21, 2019
Merged

refactor(core): use hable #6271

merged 3 commits into from
Aug 21, 2019

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Aug 21, 2019

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

jsless/hable is an extraction of our internal hooks implementation with the same tests and features. Pros of externalizing:

  • Easier maintenance
  • Using a consistent solution across different areas (I'm making a PR for using hooks in cli for example)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@pi0 pi0 requested a review from a team August 21, 2019 17:43
@pi0 pi0 added this to Ready to Review in Nuxt v2.10 Aug 21, 2019
@pi0 pi0 self-assigned this Aug 21, 2019
@pimlie
Copy link

pimlie commented Aug 21, 2019

Hable & Hookable dont have a fully similar interface and implementation

@pi0
Copy link
Member Author

pi0 commented Aug 21, 2019

@pimlie hmm I'm not sure. Can you please elaborate more about differences and potentially missing requirements?

UPDATE: Hable 2.1.0 supports custom logger (consola) and optional fatal for nuxt behaviour compatibility.

Atinux
Atinux previously approved these changes Aug 21, 2019
Copy link
Member

@Atinux Atinux left a comment

Choose a reason for hiding this comment

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

YES!

@codecov-io
Copy link

codecov-io commented Aug 21, 2019

Codecov Report

Merging #6271 into dev will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6271      +/-   ##
==========================================
- Coverage   95.76%   95.67%   -0.09%     
==========================================
  Files          80       79       -1     
  Lines        2666     2636      -30     
  Branches      687      679       -8     
==========================================
- Hits         2553     2522      -31     
- Misses         97       98       +1     
  Partials       16       16
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.26% <100%> (-0.38%) ⬇️
#unit 92.33% <100%> (-0.13%) ⬇️
Impacted Files Coverage Δ
packages/core/src/nuxt.js 94.73% <100%> (ø) ⬆️
packages/vue-renderer/src/renderer.js 93.49% <0%> (-0.82%) ⬇️

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 6ac5544...6511eef. Read the comment docs.

clarkdo
clarkdo previously approved these changes Aug 21, 2019
Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

LGTM

pimlie
pimlie previously requested changes Aug 21, 2019
Copy link

@pimlie pimlie left a comment

Choose a reason for hiding this comment

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

Hookable:

  • flatHooks as instance method (probable safe to remove though, but who knows)
  • filter(Boolean) in addHooks

Hable:

  • deprecateHook

When moving to Hable, Nuxt should use the deprecateHook method instead of setting the prop directly.

Hable will add falsy hooks in addHooks / flatHooks util. Actually it will also add all other non-function hooks, but thats also the case with Hookable.

@pi0
Copy link
Member Author

pi0 commented Aug 21, 2019

flatHooks as an instance method (probable safe to remove though, but who knows)

This is an internal utility. So I think it is safe to not include in instance.

deprecateHook

Yeah. That's an addition of hable that we can leverage :) I'll refactor to use it.

UPDATE: hable 2.2.1 has deprecateHooks util.

filter(Boolean) in addHooks

Main hook function which is called by addHooks already prevents adding falsy hooks.

@pi0 pi0 dismissed stale reviews from clarkdo and Atinux via a5c68b6 August 21, 2019 19:32
@pi0 pi0 requested a review from pimlie August 21, 2019 19:32
@pi0 pi0 merged commit 9ad02c4 into dev Aug 21, 2019
@pi0 pi0 deleted the refactor/hable branch August 21, 2019 19:46
@pi0 pi0 moved this from Ready to Review to Done in Nuxt v2.10 Aug 21, 2019
@pi0 pi0 mentioned this pull request Aug 22, 2019
Atinux pushed a commit that referenced this pull request Aug 28, 2019
* [release]

* fix(vue-app): enforce default css when used with frameworks (#6255)

* chore(deps): update dependency serialize-javascript to ^1.8.0 (#6254)

* fix(config): provide typescript backward compatibility (#6258)

* fix(vue-app): only overwrite leave listener when none provided or without done arg (#6264)

* fix(nuxt-start): add missing `vue-client-only` dependency (#6267)

* fix(server): treat `https: null` as `https: undefined` (#6265)

* chore(deps): update devdependency rollup to ^1.20.0 (#6268)

* fix: improve watching experience for generated files (#6257)

* refactor(core): use hable (#6271)

* test: disable terser/minify by default (#6290)

* chore(deps): update devdependency rollup to ^1.20.1 (#6282)

* chore(cli): accept hooks (#6274)

* chore(deps): update devdependency rollup-plugin-alias to v2 (#6281)

* fix(builder): apply overrides from app dir only (#6283)

[release]

* feat: function watchQuery (#6245)

* Revert "feat: function watchQuery (#6245)" (#6296)

This reverts commit 3c61830.

* chore(deps): update devdependency rollup to ^1.20.2 (#6295)



Co-authored-by: Renovate Bot <bot@renovateapp.com>

* test(vue-app): add template compiler helper (#6299)

* test: add jest roots for less greedy test search (#6300)

* test: remove unnecessary generate (#6301)

* fix: update the minimal require node version in distributions (#6310)

* chore(deps): update devdependency babel-eslint to ^10.0.3 (#6304)

* chore(deps): lock file maintenance (#6305)

* chore: upgrade circleci config to v2.1 (#6312)

* test: add test for modern bundle size (#6302)

* test: add client-only test to basic fixture (#6315)

test: check for no-ssr deprecation warning
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Nuxt v2.10
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants