Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Add bundle analyzer and update React imports #160

Merged
merged 4 commits into from
Aug 12, 2021
Merged

Conversation

katjuell
Copy link
Contributor

What should this PR do?

This PR resolves DEVED-106 by adding a bundle analyzer to our project and updating some of our import statements.

You can now run npm run analyze to look at the current state of the app. It will be helpful to also use tools like https://bundlephobia.com/ before adding any new packages to the app.

This PR also updates our React imports to work with current thinking, avoiding redundant imports. See also this PR

Why are we making this change?

We want our user experience to remain optimal.

What are the acceptance criteria?

  • The app works as expected.
  • The analyzer works as expected.

How should this PR be tested?

  • Check out the branch, nuke node_modules, and install dependencies with npm ci.
  • Run npm run analyze and ensure that analyzer works.
  • Run the app and check for unexpected behavior.

Pull request process

Reviewers:

  1. Test functionality using the criteria above.
  2. Offer tips for efficiency, feedback on best practices, and possible alternative approaches and things that may not have been considered.
  3. For shorter, "quick" PRs, use your best judgement on #​2.
  4. Use a collaborative approach and provide resources and/or context where appropriate.
  5. Provide screenshots/grabs where appropriate to show findings during review.

Reviewees:

  1. Prefer incremental and appropriately-scoped changes.
  2. Leave a comment on things you want explicit feedback on.
  3. Respond clearly to comments and questions.

@katjuell katjuell added the enhancement Updates to existing features label Aug 11, 2021
@katjuell katjuell self-assigned this Aug 11, 2021
@@ -5,7 +5,8 @@
"jsx-a11y/anchor-is-valid": "off",
"import/no-default-export": "off",
"react/forbid-dom-props": "warn",
"@typescript-eslint/require-await": "warn"
"@typescript-eslint/require-await": "warn",
"react/react-in-jsx-scope": "off"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that with React v17 we can disable this.

@netlify
Copy link

netlify bot commented Aug 11, 2021

✔️ Deploy Preview for sourcegraph-learn ready!

🔨 Explore the source changes: d12397b

🔍 Inspect the deploy log: https://app.netlify.com/sites/sourcegraph-learn/deploys/61146afc8b1e470007da0b7f

😎 Browse the preview: https://deploy-preview-160--sourcegraph-learn.netlify.app

@@ -61,8 +61,7 @@ const Layout: React.FunctionComponent<Props> = props => {
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<link rel="preconnect" href="https://fonts.gstatic.com" />
<link
rel="stylesheet preload prefetch"
as="style"
rel="stylesheet"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will hopefully address a console warning I was seeing in prod (but not dev while working on CSS refactor): <link rel=preload> has an invalid href value. This should fix that.

@@ -1,14 +1,14 @@
import Column from '@components/atoms/Column'
import Row from '@components/atoms/Row'
import Layout, { Props as LayoutProps, MetaTags } from '@components/layouts/Layout'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking MetaTags out here, since this is unused.

Copy link
Contributor

@ltagliaferri ltagliaferri left a comment

Choose a reason for hiding this comment

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

Analyzer and dev environment work as expected. Going to make sure that the deploy preview is also working as expected (updating branch now). Thanks for addressing the import statements too.

@ltagliaferri ltagliaferri merged commit 4e19e87 into main Aug 12, 2021
@ltagliaferri ltagliaferri deleted the kjuell/react-imports branch August 12, 2021 00:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Updates to existing features ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants