-
Notifications
You must be signed in to change notification settings - Fork 336
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
chore: put server assets on CDN #9278
Conversation
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
@@ -0,0 +1,16 @@ | |||
import appOrigin from './appOrigin' | |||
|
|||
declare let __webpack_public_path__: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't set the public path at buildtime because different PPMIs will host their assets in different places.
since the public path is defined during predeploy, we must set it at runtime
path.join(__dirname, jiraPlaceholder.slice(__webpack_public_path__.length)) | ||
) | ||
try { | ||
const res = await fetch(jiraPlaceholder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all other assets need a public URL except this one, so instead of special casing this one I just turned it into a fetch vs. a file system read.
[path.join(PROJECT_ROOT, 'static')]: !__PRODUCTION__, | ||
[path.join(PROJECT_ROOT, 'dev', 'dll')]: !__PRODUCTION__ | ||
// publish server assets at /static | ||
[path.join(PROJECT_ROOT, 'dist')]: __PRODUCTION__, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path is left intact, e.g. dist/images/imageNotFound.png
will be published at /static/images/imageNotFound.png
@@ -82,7 +78,7 @@ const rewriteIndexHTML = () => { | |||
const keys = `<script>window.__ACTION__=${JSON.stringify(clientKeys)}</script>` | |||
const rawHTML = skeleton | |||
.replace('<head>', `<head>${noindex}${keys}`) | |||
.replaceAll('__PUBLIC_PATH__', getCDNURL()) | |||
.replaceAll('__PUBLIC_PATH__', __webpack_public_path__.replace(/\/$/, '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove trailing slash
scripts/toolboxSrc/pushToCDN.ts
Outdated
@@ -2,6 +2,11 @@ import fs from 'fs' | |||
import getFileStoreManager from 'parabol-server/fileStorage/getFileStoreManager' | |||
import path from 'path' | |||
import getProjectRoot from '../webpack/utils/getProjectRoot' | |||
;(require as any).context( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the DB needs these files, we require them here so they get added to the server bundle, e.g. dist/images/templates
@@ -25,46 +30,78 @@ const pushClientAssetsToCDN = async () => { | |||
console.log(`⛅️ Uploaded ${dirEnts.length} client assets to CDN`) | |||
} | |||
|
|||
const pushTemplatesToCDN = async () => { | |||
const pushServerAssetsToCDN = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we were just pushing client assets, now we push server assets, too.
This is useful for things like:
- anonymousAvatar getting referenced in emails
- our logo's URL getting published in mattermost messages
@@ -25,8 +26,8 @@ module.exports = { | |||
__dirname: false | |||
}, | |||
entry: { | |||
web: [DOTENV, path.join(SERVER_ROOT, 'server.ts')], | |||
gqlExecutor: [DOTENV, path.join(GQL_ROOT, 'gqlExecutor.ts')] | |||
web: [DOTENV, INIT_PUBLIC_PATH, path.join(SERVER_ROOT, 'server.ts')], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the public path has to get set before imports use it, so set it here because that way the import order of server.ts doesn't matter
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
@@ -129,5 +130,5 @@ jobs: | |||
uses: actions/upload-artifact@v2 | |||
with: | |||
name: test-results | |||
path: test-results/ | |||
path: packages/integration-tests/test-results/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been wrong since forever, i just didn't ever need it until today 😞
@igor would you mind giving this a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -47,8 +45,6 @@ const writeManifest = () => { | |||
const manifestPath = path.join(clientDir, 'manifest.json') | |||
fs.writeFileSync(manifestPath, JSON.stringify(manifest)) | |||
// move the referenced icons into the client build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not moving already
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Description
Publicly host server assets so they can be used by downstream services like email, mattermost, etc.
fix #9111
fix #8553
Demo
Testing scenarios
in dev, with no CDN
in prod, with no CDN (after running preDeploy)
in prod, with CDN (after running preDeploy)