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

feat: custom 404 response via headers #35

Merged
merged 9 commits into from
Dec 20, 2021
Merged

feat: custom 404 response via headers #35

merged 9 commits into from
Dec 20, 2021

Conversation

nightnei
Copy link
Contributor

No description provided.

src/app/index.ts Outdated

if (isCustomPage) {
status.headers = {
['X-ILC-Override']: 'error-page-content',
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect to see this code with HTTP header in SSR code, rather then in AppSdk

Because before AppSdk knew nothing about server communication protocol

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it's better to work this out through adapters that AppSdk receives at CSR & SSR respectively

@nightnei nightnei force-pushed the custom404Response branch 4 times, most recently from 9a9a076 to ee2d4be Compare December 17, 2021 12:25
…equest function, and AppSdk is predefined in processRequest too
@nightnei nightnei force-pushed the custom404Response branch 3 times, most recently from 49c1781 to fedbe93 Compare December 17, 2021 12:50
@@ -0,0 +1,4 @@
export type TmpResponseData = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export type TmpResponseData = {
export type SsrContext = {

@nightnei nightnei merged commit 5515a48 into master Dec 20, 2021
@nightnei nightnei deleted the custom404Response branch December 20, 2021 08:12
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.

2 participants