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

SDK: better support for SPA routing - STEP 1/2 : add hash tracking #463

Conversation

mquentin
Copy link
Member

@mquentin mquentin commented Jul 22, 2020

Motivation

Given
startViewCollection
When
on window.location.hash changes
Then
check if the views are different.
If the hash fragment has changed, then the views are different.

Changes

  • ✨ add a trackHash function
  • ✨ modify areDifferentViews to handle hash changes

Testing

✅ hash changes tests have been added both for pushStates and window.location.hash window.location.hash
Test with the Vue router without mode: 'history':

const router = new VueRouter({ routes })

SHOULD NOT BE MERGE UNTIL STEP 2 is done

SDK: better support for SPA routing - STEP 2/2 : filter out html anchor tags changes

@mquentin mquentin requested a review from a team as a code owner July 22, 2020 09:07
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Outdated Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Show resolved Hide resolved
packages/rum/test/viewCollection.spec.ts Show resolved Hide resolved
packages/rum/src/viewCollection.ts Outdated Show resolved Hide resolved
@bcaudan
Copy link
Contributor

bcaudan commented Jul 22, 2020

This PR should not be merged on master while anchor hash are not excluded, right?

@mquentin mquentin changed the title SDK: better support for SPA routing - add hash tracking SDK: better support for SPA routing - STEP 1/2 : add hash tracking Jul 22, 2020
@mquentin mquentin changed the base branch from master to maxime.quentin/betterSupportForSPArouting July 22, 2020 14:41
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2020

Codecov Report

Merging #463 into maxime.quentin/betterSupportForSPArouting will decrease coverage by 0.42%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                              Coverage Diff                              @@
##           maxime.quentin/betterSupportForSPArouting     #463      +/-   ##
=============================================================================
- Coverage                                      87.93%   87.51%   -0.43%     
=============================================================================
  Files                                             32       32              
  Lines                                           2022     2026       +4     
  Branches                                         410      411       +1     
=============================================================================
- Hits                                            1778     1773       -5     
- Misses                                           244      253       +9     
Impacted Files Coverage Δ
packages/rum/src/viewCollection.ts 96.00% <100.00%> (+0.16%) ⬆️
packages/core/src/init.ts 82.85% <0.00%> (-5.72%) ⬇️
packages/rum/src/rum.entry.ts 71.69% <0.00%> (-5.67%) ⬇️
packages/rum/src/rum.ts 86.77% <0.00%> (-4.14%) ⬇️
packages/rum/src/parentContexts.ts 100.00% <0.00%> (+1.78%) ⬆️

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 5ad8198...52aafd5. Read the comment docs.

@mquentin mquentin requested a review from bcaudan July 27, 2020 10:33
@mquentin mquentin merged commit 57391e2 into maxime.quentin/betterSupportForSPArouting Jul 28, 2020
@mquentin mquentin deleted the maxime.quentin/betterSupportForSPAroutingAddhashTracking branch July 28, 2020 12:38
bcaudan added a commit that referenced this pull request Aug 4, 2020
* SDK: better support for SPA routing - STEP 1/2 : add hash tracking (#463)

* add hash tracking

* ✅ add hash change tests

* introduce renewViewOnChange

* add better management and comments of the areViewDifferent hash part

* set async tests for hashchanges

* reset window.location.hash of previous tests

* skip tesst using Promise for IE

* replace jasmine promise with callback process

* remove anchor check on this PR

* ✅ and implem reviews

* SDK: better support for SPA routing - STEP 2/2 : filter out html anchor tags changes (#466)

* add hash tracking

* ✅ add hash change tests

* introduce renewViewOnChange

* add better management and comments of the areViewDifferent hash part

* set async tests for hashchanges

* reset window.location.hash of previous tests

* skip tesst using Promise for IE

* replace jasmine promise with callback process

* remove anchor check on this PR

* add anchor nav filter

* patch tslint error

* ✅ and implem reviews

* implement reviews regarding func naming and hash cleaning

* simplify mockGetElementById

* add e2e tests

* simply anchor filter out proccess

* implement reviews

* unit test the hash change acknowledgement after an anchor nav

* fix typo

* fix typo v2

* fix mock hash

Co-authored-by: Bastien Caudan <bastien.caudan@gmail.com>
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