Skip to content

Refactor dfns #464

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.

Sign up for GitHub

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

Open
reddy-vishwanath wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Refactor dfns #464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c7971a
wip
reddy-vishwanath Jun 15, 2025
fb17ba5
wip
reddy-vishwanath Jun 15, 2025
46ae656
wip
reddy-vishwanath Jun 15, 2025
a42bd3c
wip
reddy-vishwanath Jun 15, 2025
4679eb6
wip
reddy-vishwanath Jun 15, 2025
4199f94
chore(release): 7.34.2-refactor-dfns.0
reddy-vishwanath Jun 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@quintype/framework",
"version": "7.34.1",
"version": "7.34.2-refactor-dfns.0",
"description": "Libraries to help build Quintype Node.js apps",
"main": "index.js",
"engines": {
Expand Down
57 changes: 47 additions & 10 deletions server/handlers/isomorphic-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ function createStoreFromResult(url, result, opts = {}) {
primaryHostUrl: result.primaryHostUrl,
isBotRequest: isBotRequest,
lazyLoadImageMargin: opts.lazyLoadImageMargin,
localeModule: _.get(opts, ['localeModule'])
};
return createBasicStore(result, qt, opts);
}
Expand Down Expand Up @@ -471,7 +472,7 @@ exports.handleIsomorphicRoute = function handleIsomorphicRoute(
) {
const url = urlLib.parse(req.url, true);

function writeResponse(result) {
function writeResponse(result, opts) {
const statusCode = result.httpStatusCode || 200;

if (statusCode == 301 && result.data && result.data.location) {
Expand All @@ -491,6 +492,7 @@ exports.handleIsomorphicRoute = function handleIsomorphicRoute(
const store = createStoreFromResult(url, result, {
disableIsomorphicComponent: statusCode != 200,
lazyLoadImageMargin,
localeModule: _.get(opts, ['localeModule'])
});

if (lightPages) {
Expand Down Expand Up @@ -553,20 +555,55 @@ exports.handleIsomorphicRoute = function handleIsomorphicRoute(
logError(e);
return { httpStatusCode: 500, pageType: "error" };
})
.then((result) => {
.then(async (result) => {
if (!result) {
return next();
}
return new Promise((resolve) => resolve(writeResponse(result)))
.catch((e) => {
logError(e);
res.status(500);
res.send(e.message);
})
.finally(() => res.end());
});
return getStoreOpts(config).then((opts) => {
return new Promise((resolve) => resolve(writeResponse(result, opts)))
.catch((e) => {
logError(e);
res.status(500);
res.send(e.message);
})
.finally(() => res.end());
});
})
Comment on lines +558 to +571
Copy link

@coderabbitai coderabbitai bot Jun 15, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Unnecessary promise-wrapping & potential double res.end()

  1. The async arrow already returns a promise. Wrapping writeResponse in new Promise(resolve => resolve(...)) is redundant noise.
  2. writeResponse ultimately writes to res; most implementations of renderLayout call res.end()/res.send().
    The outer .finally(() => res.end()) therefore risks calling res.end() twice.
-    .then(async (result) => {
+    .then((result) => {
       if (!result) {
         return next();
       }
-      return getStoreOpts(config).then((opts) => {
-        return new Promise((resolve) => resolve(writeResponse(result, opts)))
-          .catch((e) => {
-            logError(e);
-            res.status(500);
-            res.send(e.message);
-          })
-          .finally(() => res.end());
-      });
+      return getStoreOpts(config)
+        .then((opts) => writeResponse(result, opts))
+        .catch((e) => {
+          logError(e);
+          res.status(500).send(e.message);
+        });
     })

If renderLayout already closes the stream, drop the outer finally.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.then(async (result) => {
if (!result) {
return next();
}
return new Promise((resolve) => resolve(writeResponse(result)))
.catch((e) => {
logError(e);
res.status(500);
res.send(e.message);
})
.finally(() => res.end());
});
return getStoreOpts(config).then((opts) => {
return new Promise((resolve) => resolve(writeResponse(result, opts)))
.catch((e) => {
logError(e);
res.status(500);
res.send(e.message);
})
.finally(() => res.end());
});
})
.then((result) => {
if (!result) {
return next();
}
return getStoreOpts(config)
.then((opts) => writeResponse(result, opts))
.catch((e) => {
logError(e);
res.status(500).send(e.message);
});
})
🤖 Prompt for AI Agents 
In server/handlers/isomorphic-handler.js around lines 558 to 571, remove the
unnecessary wrapping of writeResponse in a new Promise since the async function
already returns a promise. Also, if writeResponse or its underlying renderLayout
implementation already calls res.end(), eliminate the outer finally block that
calls res.end() to avoid ending the response twice. Adjust the code to directly
return writeResponse with the appropriate options and handle errors without
redundant promise wrapping or double res.end() calls.

};

async function getStoreOpts(config) {
const opts = {};
const enableTimeTranslation = _.get(config, ['pbConfig', 'general', 'enableTimeTranslation'], false);
if (!enableTimeTranslation) {
return Promise.resolve(opts);
}
const languageCode = _.get(config, ['language', 'ietf-code']);
try {
let localeModule;
switch (languageCode) {
case 'hi':
localeModule = require('date-fns/locale/hi');
break;
case 'ta':
localeModule = require('date-fns/locale/ta');
break;
case 'de':
localeModule = require('date-fns/locale/de');
break;
default:
localeModule = require('date-fns/locale/en-US');
break;
}
opts.localeModule = localeModule.default || localeModule;
return Promise.resolve(opts);
} catch (err) {
console.warn(`Falling back to en-US due to error loading locale: ${languageCode}`, err);
const fallbackLocale = require('date-fns/locale/en-US');
opts.localeModule = fallbackLocale.default || fallbackLocale;
return Promise.resolve(opts);
}
}

exports.handleStaticRoute = function handleStaticRoute(
req,
res,
Expand Down