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

Refactored boot time / on demand asset generation #21866

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mike182uk
Copy link
Member

@mike182uk mike182uk commented Dec 11, 2024

refs ONC-662

Refactored the generation of the following assets:

  • comment-counts.js
  • member-attribution.js
  • admin-auth

to occur at build time instead of at boot time or on demand

cards are still built on demand as these assets need to be re-generated when a theme changes - we utilise the existing asset minification system for this, except we only execute the middleware responsible for this when a request is for a card asset (instead of any path, which is a bug in the current implementation)

This also reverts the changes made in #21857 as these changes are no longer needed (because the assets are now generated at build time)

@mike182uk mike182uk force-pushed the mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2 branch 6 times, most recently from 6cf4c9d to ceb9ad9 Compare December 12, 2024 14:56
@mike182uk mike182uk changed the title Refactored built asset generation for static assets Refactored boot time / on demand asset generation to occur at build time Dec 12, 2024
@mike182uk mike182uk force-pushed the mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2 branch 3 times, most recently from e48ee21 to 7960d6b Compare December 13, 2024 14:01
@mike182uk mike182uk changed the title Refactored boot time / on demand asset generation to occur at build time Refactored boot time / on demand asset generation Dec 13, 2024
@mike182uk mike182uk force-pushed the mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2 branch 2 times, most recently from 173a2e5 to 6ead434 Compare December 13, 2024 14:12
@mike182uk mike182uk marked this pull request as ready for review December 13, 2024 14:21
@@ -22,7 +22,9 @@
"scripts": {
"archive": "npm pack",
"dev": "node --watch index.js",
"build:assets": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css",
"build:assets:css": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css",
"build:assets:js": "../minifier/bin/minify core/frontend/public/comment-counts.js core/frontend/public/comment-counts.min.js && ../minifier/bin/minify core/frontend/public/member-attribution.js core/frontend/public/member-attribution.min.js && ../minifier/bin/minify core/frontend/public/admin-auth/message-handler.js core/frontend/public/admin-auth/admin-auth.min.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

@daniellockyer Any thoughts on this?

}, serveStatic(
path.join(config.getContentPath('public'), 'admin-auth')
));
}, createServeAuthFrameFileMw(config, urlUtils));
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to switch to using a custom middleware as express/serve-static does not allow us to hook into the response body to perform the necessary placeholder replacements

@@ -56,29 +53,3 @@ describe('vhost utils', function () {
});
});
});

describe('getContentPath', function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed as we are reverting the changes added here

@mike182uk mike182uk force-pushed the mike-onc-662-fix-file-write-issues-in-ghost-application-related-to-asset-2 branch from 6ead434 to 0d7846d Compare December 19, 2024 09:14
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.

1 participant