fix(docs-infra): do not break when cookies are disabled in the browser by ajitsinghkaler · Pull Request #33829 · angular/angular

ajitsinghkaler · 2019-11-14T18:03:29Z

fix(docs-infra): changed window.sessionStorage to default params when cookies disabled

Whenever cookies are disabled windows.sessionStorage is not available for the app so If it throws an error I give it a default value

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

In the current scenario whenever the browser is configured not to accept cookies, the angular.io homepage shows a blank page without any content because windows.sessionStorage is not defined

Issue Number: #33795

What is the new behavior?

In the new behavior whenever window.sessionStorage is not defined I give it a default value

Does this PR introduce a breaking change?

Yes
No

Other information

ajitsinghkaler · 2019-11-14T18:04:33Z

@gkalpak I have issued a pull request can you guide me through the next steps.

gkalpak

Nice!

The next step would be to add a test for this (something that would fail without your fix and passes with the fix). The unit tests for ScrollService are in aio/src/app/shared/scroll.service.spec.ts, so you could add a test somewhere here:

it('should not break when cookies are disabled in the browser', () => {
  // Simulate `window.sessionStorage` being inaccessible, when cookies are disabled.
  spyOnProperty(window, 'sessionStorage', 'get').and.throwError('The operation is insecure');

  expect(() => {
    const platformLoc = platformLocation as PlatformLocation;
    const service = new ScrollService(document, platformLoc, viewportScrollerStub, location);

    service.updateScrollLocationHref();
    expect(service.getStoredScrollLocationHref()).toBeNull();

    service.removeStoredScrollInfo();
    expect(service.getStoredScrollPosition()).toBeNull();
  }).not.toThrow();
});

As you probably know already, you can run the tests with yarn test (from inside the aio/ directory). Please, make sure that the test fails without your fix and passes with the fix.

ajitsinghkaler · 2019-11-15T17:51:35Z

@gkalpak can you please look into the request and tell me if there is a problem with my code.

gkalpak · 2019-11-18T11:54:20Z

The code looks great, @ajitsinghkaler 👍
Now, the final steps are to get CI green and fix the commit messages to more closely follow our guidelines.

Rebase your branch on master (this should help get CI green).
Squash your commits into one (there should only be one commit, since this is practically one change).
Update the commit message (which is already is a pretty good state 💯) to be a little more about what issue is fixed (and less about how we fixed it) and also mention that the commit fixes angular.io homepage doesn't load without cookies enabled #33795. I.e. something like:

fix(docs-infra): do not break when cookies are disabled in the browser

Whenever cookies are disabled in the browser, `window.sessionStorage`
is not avaialable to the app (i.e. even trying to access
`window.sessionStorage` throws an error).

To avoid breaking the app, we use a no-op `Storage` implementation if
`window.sessionStorage` is not available.

Fixes angular.io homepage doesn't load without cookies enabled #33795

mary-poppins · 2019-11-18T16:58:19Z

You can preview 7b75cf1 at https://pr33829-7b75cf1.ngbuilds.io/.

googlebot · 2019-11-18T17:13:05Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2019-11-18T17:13:06Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

mary-poppins · 2019-11-18T17:20:52Z

You can preview 4d95ce2 at https://pr33829-4d95ce2.ngbuilds.io/.

googlebot · 2019-11-18T17:23:57Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mary-poppins · 2019-11-18T17:30:57Z

You can preview c9b1619 at https://pr33829-c9b1619.ngbuilds.io/.

mary-poppins · 2019-11-18T17:49:33Z

You can preview 3e088f5 at https://pr33829-3e088f5.ngbuilds.io/.

mary-poppins · 2019-11-18T18:07:02Z

You can preview 26d5730 at https://pr33829-26d5730.ngbuilds.io/.

ajitsinghkaler · 2019-11-18T18:20:49Z

@gkalpak now I can't understand why all checks are not passing can you please help me

gkalpak · 2019-11-18T19:21:13Z

For starters, you need to rebase on master (as mentioned in my previous message).
The error on CI are a little unintuitive at the moment, but we are working on throwing a more descriptive error, when you need to rebase.

(If you are having trouble rebasing on master, lmk and I can do it for you.)

ajitsinghkaler · 2019-11-19T03:48:11Z

I've already run git rebase master in my feature branch
Should I rebase my feature branch on the master branch

gkalpak · 2019-11-19T09:37:18Z

Thx for trying that, @ajitsinghkaler. git rebase master in your feature branch does indeed rebase your branch on your (local) master, so that's should be enough. Maybe your master is not up-to-date, though.

Try the following:

# Switch to the master branch.
git checkout master

# Fetch changes from `angular/angular` and
# update your local master.
git pull upstream master

# Push the changes to your remote GitHub repo
# (This is not necessary for rebasing your local feature branch,
# but it is a good idea to keep your repo's master up-to-date.)
git push origin master

# Switch to your local feature branch.
# (Replace `<your-local-branch>` with the name of your branch.)
git checkout <your-local-branch>

# Rebase your feature branch on master.
git rebase master

# Push the updated feature branch to your remote GitHub repo.
# (This will automaticall yupdate the PR.)
git push --force-with-lease

(If you get an error about upstream not being known, run the following command first: git remote add upstream https://github.com/angular/angular.git)

Let me know if these commands don't work for you.

Whenever cookies are disabled in the browser, `window.sessionStorage` is not avaialable to the app (i.e. even trying to access `window.sessionStorage` throws an error). To avoid breaking the app, we use a no-op `Storage` implementation if `window.sessionStorage` is not available. Fixes angular#33795

mary-poppins · 2019-11-19T14:31:20Z

You can preview 6d3b739 at https://pr33829-6d3b739.ngbuilds.io/.

ajitsinghkaler · 2019-11-19T14:47:30Z

@gkalpak everything is alright I think right now
Can you please merge the branch

gkalpak

LGTM ✨
Thx, @ajitsinghkaler 💯

#33829) Whenever cookies are disabled in the browser, `window.sessionStorage` is not avaialable to the app (i.e. even trying to access `window.sessionStorage` throws an error). To avoid breaking the app, we use a no-op `Storage` implementation if `window.sessionStorage` is not available. Fixes #33795 PR Close #33829

ajitsinghkaler · 2019-11-22T16:10:18Z

@gkalpak No problem. Actually I should thank you for helping me throughout the process.

You can assign me any issue that seems fit for my knowledge. I'll try to help the angular team as much as possible.

gkalpak · 2019-11-25T11:01:48Z

That's good to know, @ajitsinghkaler, thx!
Feel free to pick anything from the hotlist: community-help list.

_{(BTW, I would love some help figuring out why there is a visual difference in different OSes in #33255 (comment), if you happen to fancy looking into that 😉)}

angular-automatic-lock-bot · 2019-12-26T16:04:40Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ajitsinghkaler requested a review from a team November 14, 2019 18:03

googlebot added the cla: yes label Nov 14, 2019

gkalpak added comp: docs-infra state: WIP target: patch This PR is targeted for the next patch release type: bug/fix labels Nov 14, 2019

ngbot Bot added this to the needsTriage milestone Nov 14, 2019

gkalpak suggested changes Nov 14, 2019

View reviewed changes

Comment thread aio/src/app/shared/scroll.service.ts Outdated

gkalpak added the aio: preview label Nov 14, 2019

ajitsinghkaler requested a review from gkalpak November 15, 2019 17:22

ajitsinghkaler force-pushed the session-storage-bug branch from 1def8e9 to 7b75cf1 Compare November 18, 2019 16:48

googlebot added cla: no and removed cla: yes labels Nov 18, 2019

ajitsinghkaler force-pushed the session-storage-bug branch from 4d95ce2 to c9b1619 Compare November 18, 2019 17:23

googlebot added cla: yes and removed cla: no labels Nov 18, 2019

ajitsinghkaler force-pushed the session-storage-bug branch 4 times, most recently from 3ae3f02 to 3e088f5 Compare November 18, 2019 17:42

ajitsinghkaler force-pushed the session-storage-bug branch from 3e088f5 to 26d5730 Compare November 18, 2019 17:57

gkalpak mentioned this pull request Nov 19, 2019

Add elements project type for boilerplate example projects #33484

Closed

14 tasks

ajitsinghkaler force-pushed the session-storage-bug branch from 26d5730 to 6d3b739 Compare November 19, 2019 14:24

gkalpak changed the title ~~fix(docs-infra): changed window.sessionStorage to default params when cookies disabled~~ fix(docs-infra): do not break when cookies are disabled in the browser Nov 19, 2019

gkalpak added action: merge The PR is ready for merge by the caretaker and removed state: WIP labels Nov 19, 2019

gkalpak approved these changes Nov 19, 2019

View reviewed changes

alxhub closed this in f69c6e2 Nov 19, 2019

ajitsinghkaler deleted the session-storage-bug branch November 19, 2019 22:51

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Dec 26, 2019

Conversation

ajitsinghkaler commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

ajitsinghkaler commented Nov 14, 2019

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajitsinghkaler commented Nov 15, 2019

Uh oh!

gkalpak commented Nov 18, 2019

Uh oh!

mary-poppins commented Nov 18, 2019

Uh oh!

googlebot commented Nov 18, 2019

Uh oh!

googlebot commented Nov 18, 2019

Uh oh!

mary-poppins commented Nov 18, 2019

Uh oh!

googlebot commented Nov 18, 2019

Uh oh!

mary-poppins commented Nov 18, 2019

Uh oh!

mary-poppins commented Nov 18, 2019

Uh oh!

mary-poppins commented Nov 18, 2019

Uh oh!

ajitsinghkaler commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak commented Nov 18, 2019

Uh oh!

ajitsinghkaler commented Nov 19, 2019

Uh oh!

gkalpak commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mary-poppins commented Nov 19, 2019

Uh oh!

ajitsinghkaler commented Nov 19, 2019

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

ajitsinghkaler commented Nov 22, 2019

Uh oh!

gkalpak commented Nov 25, 2019

Uh oh!

angular-automatic-lock-bot Bot commented Dec 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ajitsinghkaler commented Nov 14, 2019 •

edited

Loading

ajitsinghkaler commented Nov 18, 2019 •

edited

Loading

gkalpak commented Nov 19, 2019 •

edited

Loading