-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat(ssr): adds server cookie output via response #320
base: development
Are you sure you want to change the base?
Conversation
Partitioned cookies aren't supported by express yet due to a dependency on an older version of the cookie package. See expressjs/express#5275. Adding an override for a higher version of the cookie package which does support partitioned cookies fixes this.
Deletion uses the cookie setter with a past date in the expiration options. Now that the setter is updated to support server cookies as well, this will work for both client and server cookies.
Quality Gate passedIssues Measures |
When is this going to be merged? Seems like this is capable of solving some significant challenges. |
@blakeoxx I tried this and didn't work as expected. For example, setting cookie in |
@pavankjadda try out this test repo: https://github.com/blakeoxx/ngx-cookie-service-18-test. You can build and start the project with I generated this project with Angular's Additionally, something I noticed was that NG18's SSR defaults to using prerendering. I had to disable this in the builder architect configuration. It's intended behavior that SSR won't run on prerendered routes and not a bug of this code, but perhaps may be worth calling out in the README. Let me know if you need anything else or don't see expected results. |
This package seems to be highly needed within the Angular community, but I’ve noticed that this PR has been open for some time. I personally have a strong need for this functionality and tried to implement it in Angular 17, but it doesn’t seem to be working. In my setup, I’m saving the token on the server using ngx-cookie-service, but when I try to retrieve it on the client side, the token is null. @blakeoxx, do you happen to have any working demos for Angular 17 with a workflow similar to mine? In advance, thanks for all your hard work on this project—it’s much appreciated! |
I’d like to take this opportunity to share what I’m trying to achieve in my app, so other users with similar goals can benefit as well. In my setup, I have a forwarded host from
I expect the browser to load this saved data without making new requests. However, when I try to retrieve the token and configuration, they’re all Here's my server.ts for reference import 'zone.js/node';
import { APP_BASE_HREF } from '@angular/common';
import { CommonEngine } from '@angular/ssr';
import * as express from 'express';
import { existsSync } from 'node:fs';
import { join } from 'node:path';
import AppServerModule from './src/main.server';
import { REQUEST as SSR_REQUEST } from 'ngx-cookie-service-ssr';
// The Express app is exported so that it can be used by serverless Functions.
export function app(): express.Express {
const server = express();
const distFolder = join(process.cwd(), 'dist/tennistool/browser');
const indexHtml = existsSync(join(distFolder, 'index.original.html'))
? join(distFolder, 'index.original.html')
: join(distFolder, 'index.html');
const commonEngine = new CommonEngine();
server.set('view engine', 'html');
server.set('views', distFolder);
// Example Express Rest API endpoints
// server.get('/api/**', (req, res) => { });
// Serve static files from /browser
server.get(
'*.*',
express.static(distFolder, {
maxAge: '1y',
})
);
// All regular routes use the Angular engine
server.get('*', (req, res, next) => {
const { protocol, originalUrl, baseUrl, headers } = req;
commonEngine
.render({
bootstrap: AppServerModule,
documentFilePath: indexHtml,
url: `${protocol}://${headers.host}${originalUrl}`,
publicPath: distFolder,
providers: [
{ provide: APP_BASE_HREF, useValue: baseUrl },
{ provide: SSR_REQUEST, useValue: req },
{ provide: 'RESPONSE', useValue: res },
// harcoded host for testing
{
provide: 'X_FORWARDED_HOST',
useValue: 'sub.mydomain.com',
},
],
})
.then((html) => res.send(html))
.catch((err) => next(err));
});
return server;
}
function run(): void {
const port = process.env['PORT'] || 4000;
// Start up the Node server
const server = app();
server.listen(port, () => {
console.log(`Node Express server listening on http://localhost:${port}`);
});
}
// Webpack will replace 'require' with '__webpack_require__'
// '__non_webpack_require__' is a proxy to Node 'require'
// The below code is to ensure that the server is run only when not requiring the bundle.
declare const __non_webpack_require__: NodeRequire;
const mainModule = __non_webpack_require__.main;
const moduleFilename = (mainModule && mainModule.filename) || '';
if (moduleFilename === __filename || moduleFilename.includes('iisnode')) {
run();
}
export default AppServerModule; |
Thanks for your kind words, @betojsx ! Just glad to help :)
This PR is intended for use with NG18 projects. I'm assuming there are some compatibility issues between the Angular internals. Have you tried using the v17 artifact referenced in this comment? If that doesn't resolve your issue, let me know and I'll be happy to create a demo repo for v17 as well. |
Yep! I forgot to mention that I already have the v17 artifact in my project. "ngx-cookie-service-ssr": "github:blakeoxx/ngx-cookie-service-ssr-artifact#v17" I really appreciate your effort to help. At this point, I’ve tried everything I could but still haven’t figured out how to make it work, so I’ll wait for some examples |
Looking over your
|
OMG @blakeoxx, you solved it! That was exactly the issue! I can’t thank you enough. 🙏 I spent countless hours debugging this, even put the project on hold for a while out of frustration. I’m so glad I decided to share it here, and even more thankful for your help. You’ve saved me a ton of time! |
@pavankjadda any updates on your review of this PR? Anything needed to keep this moving? |
Haven't got time. Will try to do this week. |
Hey, anything new on that? |
i wonder if we add code like below and access this.rawCookie instead of this.document.cookie get rawCookie(): string | undefined {
if (this.documentIsAccessible) {
return this.document.cookie;
} else {
const responseCookies = this.response?.get('Set-Cookie');
let respCookies = '';
if (responseCookies) {
respCookies = Array.isArray(responseCookies) ? responseCookies.join(';') : responseCookies;
return this.request?.headers.cookie + ';' + respCookies;
} else {
return this.request?.headers.cookie;
}
}
} We probably only need change Set method. |
@maxisam I think one potential issue I see with this code is the case of the cookie being set in the request as well as response. In this code, the cookie would be duplicated, causing the first instance from the request to be the value always returned by |
Client cookies are included in the returned value of `getCombinedCookies`, so there's no need to separate out client and server logic.
Quality Gate passedIssues Measures |
That is a good point. Do you think reverse this will solve this issue? If we have duplicated cookie, I think the response one will overwrite the request one in the next request. respCookies + ';' + this.request?.headers.cookie Don't get me wrong, I think using map is fine. My first thought was using Map to handling cookies as well, but if we add "string to map" solution, it seems like we have duplicated way to handle cookie. I feel like it is easier to maintain if we can consolidate this into one solution. Either way is fine I think. |
I do think switching the order would fix the issue. However, I'd caution that it might be surprising behavior to future maintainers that cookies appear multiple times in the string and only the first occurrence is used due to the specific coding of In the end, I also don't feel that strongly about either implementation. If you feel it would ease maintainability, I'll happily update. Do you think the maintainability boost your solution offers is worth the surprising behavior? |
well, this will be @pavankjadda 's decide. Either way is fine with me, but i do think having one solution is probably better than having both. |
This brings the fix/feature completed in #315 to the v18 codebase and resolves #266 . It increases the
SsrCookieService
s ability to use the request and response objects provided by Express to read and write cookies on the server side. This also means that cookies written during SSR will be returned to the client in the response headers.This PR also includes less refactors than the previous one, per maintainer request. The only refactors included in this PR were strictly necessary for enabling this feature.