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

[NO JIRA]: Disable loadable in CSR build #156

Merged
merged 8 commits into from
Feb 16, 2023
Merged

Conversation

jaysonwu991
Copy link

@jaysonwu991 jaysonwu991 commented Feb 7, 2023

Context

After SSR build feature was integrated into fork branch and beta versions were removed, there are some issues exist for CSR build, but it didn't happen once integration because some microsites/tools at Skyscanner weren't up-to-date with the latest version that had SSR build feature.
js-tag-manager upgraded BRS to the latest version just a few months ago, and it caused an ILD after upgrading, but when it downgraded to BRS v9, the ILD issue disappeared, so it was obvious that SSR build integration caused this issue.
@samstradling tried to fixed this issue, he raised a PR #155 to do something about the configuration of loadable, but the changes might have impact with SSR build because it did use loadable.
To solve this issue, better to disable loadable in CSR build as-was (Before SSR build integration), it is safe for us to do this because it won't break anything to BRS, it means engineers won't need to do anything for this change.

Change List

  • Disable loadable in CSR build
  • Remove unused require statement

@@ -1,6 +1,5 @@
"use strict";

const postcssNormalize = require("postcss-normalize");
Copy link
Author

Choose a reason for hiding this comment

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

This module is unused, just remove it.

Comment on lines +3 to +5
## 10.1.0

- Disable `loadable` in CSR build
Copy link
Author

Choose a reason for hiding this comment

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

Even though it is a big change to CSR build, but no any breaking is here for BRS, so a minor version is OK.

@runmoore
Copy link

Thanks @jaysonwu991 , have you tested this locally with the impacted microsite & libs to make sure it fixes the issue?

@jaysonwu991
Copy link
Author

Thanks @jaysonwu991 , have you tested this locally with the impacted microsite & libs to make sure it fixes the issue?

Hi @runmoore , yep, have tested, the issue is gone and no breaking to the microsites that use CSR build.

@runmoore
Copy link

Thanks @jaysonwu991 , this is a nice solution to the problem

@runmoore runmoore merged commit 481b06d into fork Feb 16, 2023
@runmoore runmoore deleted the disable-loadable-in-csr branch February 16, 2023 09:08
xalechez pushed a commit that referenced this pull request Apr 20, 2023
* disable loadable in CSR build
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.

2 participants