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

Adds cookies support for Angular Universal #177

Closed
wants to merge 11 commits into from
Closed

Adds cookies support for Angular Universal #177

wants to merge 11 commits into from

Conversation

pavankjadda
Copy link
Collaborator

@pavankjadda pavankjadda commented Oct 27, 2021

In order to better understand this, I went through express.js tutorial. The server.ts has some basic express.js code. After debugging for few hours, I was able to identify the issue. Here is the summary

  • When express server loads web page, it does have cookies in request object, but since our cookie service depends on document object we can't access them
  • To fix this, first we need to provide REQUEST in providers. We can achieve this by modifying server.ts file
...............

  server.get('*', (req, res) => {
    res.render(indexHtml, {
      req, providers: [
        {provide: APP_BASE_HREF, useValue: req.baseUrl},
        {provide: 'REQUEST', useValue: (req)},
        {provide: 'RESPONSE', useValue: (res)}]
    });
  });
  return server;
};

...............
  • Then we need to inject REQUEST object in cookie service, which helps us to get cookies on server side through REQUEST
import {REQUEST} from '@nguniversal/express-engine/tokens';

@Injectable()
export class CookieService {
constructor(@Optional() @Inject(REQUEST) private request: Request) {
  }

  • Then we need provide this in AppServerModule
@NgModule({
  imports: [
    AppModule,
    ServerModule,
  ],
  bootstrap: [AppComponent],
  providers: [CookieService]
})
export class AppServerModule {
}

  • Finally, if the platform is server, we can get the cookies in the following way, otherwise we can use existing method
const result: RegExpExecArray = regExp.exec(this.isServer ? this.request?.headers.get('cookie') : this.document.cookie);

I checked this in my project and it works. Let me know your thoughts on this

@pavankjadda pavankjadda self-assigned this Oct 27, 2021
@pavankjadda pavankjadda changed the title adds cookies support for Angular Universal Adds cookies support for Angular Universal Oct 27, 2021
@@ -85,7 +88,7 @@ export class CookieService {
name = encodeURIComponent(name);

const regExp: RegExp = CookieService.getCookieRegExp(name);
const result: RegExpExecArray = regExp.exec(this.document.cookie);
const result: RegExpExecArray = regExp.exec(this.isServer ? this.request?.headers.get('cookie') : this.document.cookie);
Copy link
Owner

Choose a reason for hiding this comment

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

👍

Copy link
Owner

Choose a reason for hiding this comment

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

same condition goes to getAll

@stevermeister
Copy link
Owner

@pavankjadda that's good!
I had in mind to create our own InjectionToken, but you are totally right - it's already there - REQUEST

the rest looks exactly how I had in mind.
let's update the getAll method, docs, and merge?

@pavankjadda
Copy link
Collaborator Author

@stevermeister one other small thing. I do see other cookie management library ngx-cookie in Angular world. It would be nice if they can merge with us and have single repo for all Angular users.

@stevermeister
Copy link
Owner

@pavankjadda I've sent them a message - salemdar/ngx-cookie#393

@stevermeister
Copy link
Owner

@pavankjadda let's finalize and merge this one? or still have any doubts?

@pavankjadda
Copy link
Collaborator Author

@pavankjadda let's finalize and merge this one? or still have any doubts?

When cookie expires, the website loads forever. I will test it further and let you know.

@stevermeister
Copy link
Owner

@pavankjadda we only proxy cookies from the request, right? we don't cache them so it should not be any issues with expiration time

@pavankjadda
Copy link
Collaborator Author

pavankjadda commented Nov 2, 2021

@pavankjadda we only proxy cookies from the request, right? we don't cache them so it should not be any issues with expiration time

Exactly. It shouldn't be. I will do final tests and merge the PR.

@pavankjadda
Copy link
Collaborator Author

pavankjadda commented Nov 2, 2021

@pavankjadda we only proxy cookies from the request, right? we don't cache them so it should not be any issues with expiration time

@stevermeister Can you give me access to NPM Repository? I couldn't publish beta version through Github actiona. My Npm id is jpavanaryan

@stevermeister
Copy link
Owner

@pavankjadda sure thing, added

@pavankjadda
Copy link
Collaborator Author

@pavankjadda sure thing, added

Thanks.

@pavankjadda pavankjadda marked this pull request as ready for review November 2, 2021 17:22
@pavankjadda
Copy link
Collaborator Author

@stevermeister this is ready for review. We can merge this PR. Few things to note

  • Treat this as foundation PR for SSR. Bug fixes and improvements can be added later
  • I only added get() method as of right now, we can getAll() and other methods in future
  • We still need to update docs
  • I think we should announce this only when everything is ready and we are confident that everyone can use.

Let me know your thoughts

@stevermeister
Copy link
Owner

@pavankjadda

agree.
only I think we can add getAll() now, within this PR, if you don't see a lot of hidden complexity here

@pavankjadda
Copy link
Collaborator Author

Done. Updated getAll() as well.

@pavankjadda pavankjadda deleted the ssr-cookie-service branch November 12, 2021 22:59
@stevermeister
Copy link
Owner

Hi @pavankjadda,

could you please update me with the status of it?
I thought we already agreed on everything and decided to merge it.
I just found some time to return back to this and create a nice code example and also probably update some docs.

@johanchouquet
Copy link

Hi @pavankjadda and @pavankjadda ,

Would this PR be ported back on ngx-cookie-service v12 or only available for v13 ?
I'm indeed very much interested by this.

Thank you in advance for this!

@Spawnrad
Copy link

Spawnrad commented Dec 1, 2021

Hi,

I'm interested too. Any news about this merge?

Thanks <3

@pavankjadda
Copy link
Collaborator Author

Hi @pavankjadda and @pavankjadda ,

Would this PR be ported back on ngx-cookie-service v12 or only available for v13 ? I'm indeed very much interested by this.

Thank you in advance for this!

I don't see why not. But I will have to look into it.

@pavankjadda
Copy link
Collaborator Author

Looks like this PR was accidentally closed and the branch was deleted. Created new PR #199 with same changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants