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

Next.js + Vercel! ▲ #1

Draft
wants to merge 48 commits into
base: latest
Choose a base branch
from
Draft

Next.js + Vercel! ▲ #1

wants to merge 48 commits into from

Conversation

manovotny
Copy link
Collaborator

@manovotny manovotny commented Aug 5, 2022

@vercel
Copy link

vercel bot commented Aug 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
simorgh ✅ Ready (Inspect) Visit Preview Aug 23, 2022 at 7:04PM (UTC)

@@ -1 +1 @@
v12.18.4
14
Copy link
Collaborator Author

@manovotny manovotny Aug 17, 2022

Choose a reason for hiding this comment

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

The move to Next.js didn't require this. Running anything below Node 14 is painful on an M1 Mac, so it was bumped up.

.env.local Outdated

NEXT_PUBLIC_SIMORGH_CSP_REPORTING_ENDPOINT=https://ws.bbc-reporting-api.app/report-endpoint

NEXT_PUBLIC_SIMORGH_OPTIMIZELY_SDK_KEY=LptPKDnHyAFu9V12s5xCz
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of these env files moved from an envConfig folder, got renamed to work with Next.js's dotenv convention, and all variables used on the client got prefixed with NEXT_PUBLIC_.

{children}
</Helmet>
</Head>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved non-dynamic meta and links to _app.js.

@@ -1,3 +1,3 @@
/* eslint-disable import/prefer-default-export */

export scriptPropType from './scripts';
export { default as scriptPropType } from './scripts';
Copy link
Collaborator Author

@manovotny manovotny Aug 17, 2022

Choose a reason for hiding this comment

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

This was angry... In order to properly re-export an export, they needed to be wrapped in defaults's.

export { default as vietnamese } from './svgs/vietnamese';
export { default as weather } from './svgs/weather';
export { default as yoruba } from './svgs/yoruba';
export { default as zhongwen } from './svgs/zhongwen';
Copy link
Collaborator Author

@manovotny manovotny Aug 17, 2022

Choose a reason for hiding this comment

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

These were angry... In order to properly re-export an export, they needed to be wrapped in defaults's.

};

this.verbose = (event, message) => {
fileLogger.log({ file, event, message });
// console.log({ file, event, message });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turned all of these into noops because:

  1. It was super noisy while doing development. It was hard to debug issues around all the logging.
  2. It was also logging to the filesystem via fs, which doesn't really work in Vercel land. If we really want to do this right, we'd setup a log drain.


return config;
},
};
Copy link
Collaborator Author

@manovotny manovotny Aug 19, 2022

Choose a reason for hiding this comment

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

I know we wanted to do as little custom configuration here as possible, but there's a few things going on here that are unavoidable without massive changes to the project.

  1. Simorgh uses Emotion for styling, so that needs to be enabled.
  2. Simorgh uses custom import aliases (ie. import ArticlePage from '#pages/ArticlePage/ArticlePage') to avoid dot pathing everything, so the Webpack config is enhanced to accommodate this.
  3. MomentTimezoneInclude is a custom Webpack plugin written by the SImorgh team to limit the size of timezones included to reduce bundle size.
  4. And MomentTimezoneInclude is why we need to disable lint during builds. It's a cyclical problem. We need Webpack to build the app, so the custom MomentTimezoneInclude runs to create the tz directory with the outputted timezones that are imported by other files, but we cannot build the app until it passes lint because imports fail to find tz files. This existing process would need to be modified to run before lint and before build (possibly as a separate script).

@manovotny
Copy link
Collaborator Author

If anyone is looking to run this PR / branch locally, you'll need to create a .env file at the root of the project and use the values below since I moved environment variables to Vercel and you don't have access to pull them from Vercel. 😉

# Created by Vercel CLI
VERCEL="1"
VERCEL_ENV="development"
VERCEL_URL=""
VERCEL_GIT_PROVIDER=""
VERCEL_GIT_PREVIOUS_SHA=""
VERCEL_GIT_REPO_SLUG=""
VERCEL_GIT_REPO_OWNER=""
VERCEL_GIT_REPO_ID=""
VERCEL_GIT_COMMIT_REF=""
VERCEL_GIT_COMMIT_SHA=""
VERCEL_GIT_COMMIT_MESSAGE=""
VERCEL_GIT_COMMIT_AUTHOR_LOGIN=""
VERCEL_GIT_COMMIT_AUTHOR_NAME=""
NEXT_PUBLIC_SIMORGH_CONFIG_TIMEOUT_SECONDS="5"
NEXT_PUBLIC_SIMORGH_CONFIG_CACHE_MAX_AGE_SECONDS="60"
NEXT_PUBLIC_SIMORGH_CONFIG_CACHE_ITEMS="100"
NEXT_PUBLIC_SIMORGH_OPTIMIZELY_SDK_KEY="LptPKDnHyAFu9V12s5xCz\n"
NEXT_PUBLIC_SIMORGH_CSP_REPORTING_ENDPOINT="https://ws.bbc-reporting-api.app/report-endpoint"
NEXT_PUBLIC_SIMORGH_WEBVITALS_DEFAULT_SAMPLING_RATE="100"
NEXT_PUBLIC_SIMORGH_WEBVITALS_REPORTING_ENDPOINT="https://europe-west1-bbc-otg-traf-mgr-bq-dev-4105.cloudfunctions.net/report-endpoint"
NEXT_PUBLIC_SIMORGH_ICHEF_BASE_URL="https://ichef.bbci.co.uk"
NEXT_PUBLIC_SIMORGH_INCLUDES_BASE_AMP_URL="https://news.test.files.bbci.co.uk"
NEXT_PUBLIC_SIMORGH_INCLUDES_BASE_URL="https://www.test.bbc.com/ws/includes"
NEXT_PUBLIC_SIMORGH_CONFIG_URL="https://config.test.api.bbci.co.uk/"
NEXT_PUBLIC_SIMORGH_ATI_BASE_URL="https://logws1363.ati-host.net?"

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