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

fix: prepend router.base when sending page track events #20

Merged
merged 1 commit into from Mar 17, 2020

Conversation

rchl
Copy link
Member

@rchl rchl commented Mar 17, 2020

  • include router's base in pageUrl property of triggered page tracking events
  • replace head function in example nuxt.config.js with a plain object
    as that has triggered a noisy warning and wasn't tested anyway
  • add @types/jest to provide completions for test functions (when using
    relevant language server).

@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #20 into master will decrease coverage by 5.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
- Coverage   94.44%   89.18%   -5.26%     
==========================================
  Files           2        2              
  Lines          36       37       +1     
  Branches       10       10              
==========================================
- Hits           34       33       -1     
- Misses          2        4       +2
Impacted Files Coverage Δ
lib/module.js 88.57% <100%> (-5.55%) ⬇️

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 c062258...a251bf7. Read the comment docs.

lib/plugin.js Outdated
@@ -1,5 +1,7 @@
const _layer = '<%= options.layer %>'
const _id = '<%= options.id %>'
// Remove trailing slash to avoid duplicate slashes when appending route path
const _routerBase = '<%= options.routerBase.replace(/\/+$/, '') %>'
Copy link
Member

@pi0 pi0 Mar 17, 2020

Choose a reason for hiding this comment

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

Should we consider trailingSlash here? (also generating final URL)

Also, I think we can simply do normalization from module. (probably line before return)

Copy link
Member Author

@rchl rchl Mar 17, 2020

Choose a reason for hiding this comment

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

As we discussed online, the trailingSlash option should not affect this logic.

There is a small issue with trailingSlash option that is unrelated to this PR though. I'll create an issue for it. Edit: #21

 - include router's base in `pageUrl` property of triggered page tracking events
 - replace head function in example nuxt.config.js with a plain object
   as that has triggered a noisy warning and wasn't tested anyway
 - add @types/jest to provide completions for test functions (when using
   relevant language server).
@pi0
Copy link
Member

pi0 commented Mar 17, 2020

@rchl Would you please check failing test?

@rchl
Copy link
Member Author

rchl commented Mar 17, 2020

@pi0 All tests have passed by looking at the test output. It seems to be an issue with Nuxt blocking the event loop sometimes. Could you try re-running workflow? It might just pass next time.

@pi0 pi0 changed the title fix: take into account router.base when reporting page tracking event fix: prepend router.base when sending page track events Mar 17, 2020
@pi0 pi0 merged commit 2940d85 into nuxt-community:master Mar 17, 2020
@rchl rchl deleted the fix/base-url branch March 17, 2020 14:18
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

2 participants