Skip to content

Conversation

@AlexanderFSP
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows Conventional Commits
  • 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
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@MarsiBarsi
Copy link
Contributor

Hi Alexander!
Thank you for your Pull Request, we'll take a look soon 👍

export const COOKIE = new InjectionToken<string>(
'An abstraction over document.cookie string',
{
factory: () => inject(DOCUMENT).cookie,
Copy link
Member

Choose a reason for hiding this comment

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

My only concern is in Universal it might be undefined. Haven't checked that, but maybe we can just write inject(DOCUMNT).cookie || ''? And cover that by a test.

@AlexanderFSP
Copy link
Author

Hi, sorry for the long response.
I have some points, which I would like to clarify.

  1. If it'll be realized as I did it initially - this cookie token will holds the primitive string value and wouldn't reflect changes automatically. And seems to me, this token will be pretty useless) So, I have a bit changed the implementation to fix this issue. Of course, the realization of this service can be like as ngx-cookie-service provides, but I think it's not needed for now... So, what do you think about this approach?

image

  1. Another interesting which I've found, that if we try to get cookie value from the server side - we got a NotYetImplemented error (from the domino). So, this thing should be wrapped with try-catch (as I did it above).

image

@waterplea
Copy link
Member

What is the point of it then? You inject an object, you get/set its property. It's the same amount of code as just working with DOCUMENT :)

@AlexanderFSP
Copy link
Author

AlexanderFSP commented Dec 4, 2020

@waterplea , yup) But if you do it like this, the application will crash (if we are talking about universal).. And my wish was initially to have kinda abstraction over cookie string for all platforms (for SSR create a new token in ng-web-apis/universal package which extracts cookie from the request headers & sets updated cookies in the response headers). That's my idea)

@splincode splincode closed this May 2, 2023
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.

4 participants