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.

Sign up for GitHub

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

Jump to bottom

[Next.js][Multi-site] Dynamic site resolver #1224

Merged
merged 3 commits into from
Nov 4, 2022
Merged

[Next.js][Multi-site] Dynamic site resolver #1224

merged 3 commits into from
Nov 4, 2022

Conversation

illiakovalenko
Copy link
Contributor

@illiakovalenko illiakovalenko commented Nov 1, 2022 •
edited
Loading

Description / Motivation

  • Introduced SiteResolver
  • Returns siteNamed based on the provided hostName and siteInfo list
  • Returns fallback default/custom site name when appropriate siteInfo is not found

Testing Details

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@illiakovalenko illiakovalenko requested a review from a team November 1, 2022 09:12
@art-alexeyenko
Copy link
Contributor

Not a review comment, more of a question: do we decide to ignore site language and other params during site resolving for this POC?

art-alexeyenko
art-alexeyenko requested changes Nov 2, 2022
View reviewed changes
packages/sitecore-jss/src/site/site-resolver.ts
* @returns {string} siteName
*/
static resolve = (hostName: string, sitesInfo: SiteData[], fallbackSiteName?: string): string => {
const siteInfo = sitesInfo.find((info) => info.hostName === hostName);
Copy link
Contributor

Choose a reason for hiding this comment

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

hostname can have a wildcard ("*") value, we need to account for that

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean sitesInfo.hostname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how it should be resolved in this case? which sitename should be returned, or fallback should be provided?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to clarify what is expected and supported (with extended teams) before we go further here. Specifically, regarding special tokens (wildcard, other REGEX chars?) and multi-value (e.g. "host1.com|host2.com").

ambrauer
ambrauer reviewed Nov 2, 2022
View reviewed changes
packages/sitecore-jss/src/site/site-resolver.ts Outdated
* @param {string} [fallbackSiteName] siteName to be returned in case siteName is not found
* @returns {string} siteName
*/
static resolve = (hostName: string, sitesInfo: SiteData[], fallbackSiteName?: string): string => {
Copy link
Contributor

@ambrauer ambrauer Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was expecting the site resolver would perform the fetching of the SiteData[] as well (as opposed to a param) which should be possible once the GraphQLSiteInfoService is merged from this PR. However, I can see the benefit to keeping these separate.

What do you think @art-alexeyenko? Probably a larger discussion we should have... Or something we can always adjust as POC moves along.

Copy link
Contributor Author

@illiakovalenko illiakovalenko Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I chose this approach, cause I thought that site-resolver can be used in multiple places, and it should be similar to existing pos-resolver for example

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why it couldn't still be used in multiple places? And even pos-resolver does the "fetching" (granted, it's an env var in this case) of the pos lookup data.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see much use in anything from SiteData/SiteInfo other than the site name, after it was resolved. Every other piece of data (hostname, language etc) that we may have there would already be known and serve as input into the resolve method.

ambrauer
ambrauer approved these changes Nov 4, 2022
View reviewed changes
Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

Approving for now. As discussed, we can continue in follow-up items.

@ambrauer ambrauer merged commit ca9ae90 into feature/multi-site-poc Nov 4, 2022
@ambrauer ambrauer deleted the feature/556843 branch November 4, 2022 15:12
ambrauer added a commit that referenced this pull request Jan 3, 2023
@ambrauer @art-alexeyenko @illiakovalenko
[Next.js][Multi-site] Fetch site info during build (#1276)
96bd727 
* GraphQL site info service (#1227)

* GraphQL site info service

* some tweaks

* more tweaks: debug, query

* query fix for xm cloud

* move template id to contstants

* [Next.js][Multi-site] Dynamic site resolver (#1224)

* [Next.js][Multi-site] Dynamic site resolver

* Use SiteInfo from graphql-siteinfo-service

Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>

* Plugin approach for JSS config generation. Added multisite plugin to set the "sites" prop.

* Standardized comments in config plugins. Misc fixes.

* Fixed tests and relocated headlessSiteGroupingTemplate constant (since this query is temp anyway)

* Moved defaultLanguage of 'en' to defaultConfig

* Simplify config plugin order. Delete multisite-sample.ts

Co-authored-by: Artem Alexeyenko <ala@sitecore.net>
Co-authored-by: Illia Kovalenko <23364749+illiakovalenko@users.noreply.github.com>
ambrauer added a commit that referenced this pull request Jan 12, 2023
@ambrauer @illiakovalenko
@art-alexeyenko @addy-pathania @sc-ruslanmatkovskyi
[Next.js] Multi-site implementation (#1288)
c4c7cd6 
* [Next.js][Multi-site] Create plugin approach for extract-path

* Minor changes to JSDoc

* [Next.js][Multi-site] Dynamic site resolver (#1271)

* [Next.js][Multi-site] Dynamic site resolver

* add changes

* Add unit tests, parse pattern

* etc

* simplify reg exp

* [Next.js][Multi-site] Multi-site path utils (#1275)

* [Next.js][Multi-site] Multi-site path utils

* Add return type to `getSiteRewriteData`

Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>

* [Next.js][Multi-site] Fetch site info during build (#1276)

* GraphQL site info service (#1227)

* GraphQL site info service

* some tweaks

* more tweaks: debug, query

* query fix for xm cloud

* move template id to contstants

* [Next.js][Multi-site] Dynamic site resolver (#1224)

* [Next.js][Multi-site] Dynamic site resolver

* Use SiteInfo from graphql-siteinfo-service

Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>

* Plugin approach for JSS config generation. Added multisite plugin to set the "sites" prop.

* Standardized comments in config plugins. Misc fixes.

* Fixed tests and relocated headlessSiteGroupingTemplate constant (since this query is temp anyway)

* Moved defaultLanguage of 'en' to defaultConfig

* Simplify config plugin order. Delete multisite-sample.ts

Co-authored-by: Artem Alexeyenko <ala@sitecore.net>
Co-authored-by: Illia Kovalenko <23364749+illiakovalenko@users.noreply.github.com>

* [Next.js][Multi-site] Site resolution in API routes (#1277)

* add site resolution in api routes

* re-exported SiteResolver from nextjs package

* passed siteInfo to siteResolver

* refactored based on changes in resolver function

* [Next.js][Multi-site] Site resolution in page props factory (#1281)

* SiteResolver.resolve updates: removed 'language' from site resolution logic, return SiteInfo instead of site name

* Another refactor of SiteResolver. It now must be instantiated with the array of SiteInfo passed. Also introduced a way to resolve by site name ("getByName"). The old "resolve" became "getByHost".

* Added "site" prop to page props and resolve for both base nextjs initializer (single site) and nextjs-multisite add-on.

* misc lint fixes

* Adjustments to API routes based on latest SiteResolver changes

* Added comments

* Removed generateConfig from fetch-graphql-introspection-data.ts. No longer works after config generation refactor, and not necessary anyway since we provide details on how to generate config in error message anyway

* Update page props factory error-pages plugin to use props.site.name

* [Next.js][Multi-site] Multi-site middleware plugin (#1279)

* jss-cli unit test coverage: first batch

* jss-cli unit test coverage: second batch

* jss-cli unit tests: second batch

* fix expected output for test not to fail across environments

* lint

* dev-tools unit test coverage first batch

* dev-tools unit test coverage: second batch

* resolve-scjssconfig test placeholder (for the future!)

* exclude index file from jss-vue test coverage (#1266)

* version v21.1.0-canary.58 [skip ci]

* fix test for scjssconfig resolving

* Update packages/sitecore-jss-dev-tools/src/manifest/manifest-manager.test.ts

Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>

* Update packages/sitecore-jss-dev-tools/src/manifest/manifest-manager.test.ts

Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>

* lint

* adding test data for scjssconfig

* some improvements to reject logic in resolve scjssconfig

* lint numero 2

* version v21.1.0-canary.59 [skip ci]

* #556667: fixed urls for sitemap

* version v21.1.0-canary.60 [skip ci]

* final batch

* re-gen yarn.lock

* yarn.lock re-update

* version v21.1.0-canary.61 [skip ci]

* #552985: fixed header styles

* version v21.1.0-canary.62 [skip ci]

* #546298: fixed style for showing hidden components

* version v21.1.0-canary.63 [skip ci]

* SiteResolver.resolve updates: removed 'language' from site resolution logic, return SiteInfo instead of site name

* [Next.js][Multi-site] Multi-site middleware plugin

* #559044: fixed rendering dynamic placeholder (#1278)

* version v21.1.0-canary.64 [skip ci]

* Adjust with latest site resolver changes

* adjust

* Add latest changes

* Extra comment

* extra fix

* Extend unit tests

* Revert cookie set change

* Use response cookies instead of request

* Adjust changes according to review

* Adjust changes according to review

* lint fix

Co-authored-by: Artem Alexeyenko <ala@sitecore.net>
Co-authored-by: Automated Build <builds@sitecore.com>
Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>
Co-authored-by: Ruslan Matkovskyi <rusm@sitecore.net>
Co-authored-by: Ruslan Matkovskyi <100142572+matkovskyi@users.noreply.github.com>

* [Next.js][Multi-site] Support for "sc_site" query string parameter (#1283)

* jss-cli unit test coverage: first batch

* jss-cli unit test coverage: second batch

* jss-cli unit tests: second batch

* fix expected output for test not to fail across environments

* lint

* dev-tools unit test coverage first batch

* dev-tools unit test coverage: second batch

* resolve-scjssconfig test placeholder (for the future!)

* exclude index file from jss-vue test coverage (#1266)

* version v21.1.0-canary.58 [skip ci]

* fix test for scjssconfig resolving

* Update packages/sitecore-jss-dev-tools/src/manifest/manifest-manager.test.ts

Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>

* Update packages/sitecore-jss-dev-tools/src/manifest/manifest-manager.test.ts

Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>

* lint

* adding test data for scjssconfig

* some improvements to reject logic in resolve scjssconfig

* lint numero 2

* version v21.1.0-canary.59 [skip ci]

* #556667: fixed urls for sitemap

* version v21.1.0-canary.60 [skip ci]

* final batch

* re-gen yarn.lock

* yarn.lock re-update

* version v21.1.0-canary.61 [skip ci]

* #552985: fixed header styles

* version v21.1.0-canary.62 [skip ci]

* #546298: fixed style for showing hidden components

* version v21.1.0-canary.63 [skip ci]

* SiteResolver.resolve updates: removed 'language' from site resolution logic, return SiteInfo instead of site name

* [Next.js][Multi-site] Multi-site middleware plugin

* #559044: fixed rendering dynamic placeholder (#1278)

* version v21.1.0-canary.64 [skip ci]

* Adjust with latest site resolver changes

* adjust

* Add latest changes

* Extra comment

* extra fix

* Extend unit tests

* Revert cookie set change

* Use response cookies instead of request

* Add querystring support

* Adjust changes according to review

* Adjust changes

* Adjust changes according to review

* lint fix

* Use `sc_site` request cookie when it's present

Co-authored-by: Artem Alexeyenko <ala@sitecore.net>
Co-authored-by: Automated Build <builds@sitecore.com>
Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>
Co-authored-by: Ruslan Matkovskyi <rusm@sitecore.net>
Co-authored-by: Ruslan Matkovskyi <100142572+matkovskyi@users.noreply.github.com>

* Import SiteResolver from `middleware` submodule

* Ensure site and variant prefixes are position agnostic (#1286)

* personalize rewrite unit tests

(cherry picked from commit 3c412ed6855d60452e1e29d21a92d1901312d3d4)

* site path utils unit tests, tweak to personalize path unit test

* [MultiSite] SSG support (#1284)

* added ssg support for multisite

* added changes from review comments

* added sitename for debug log

* refactored sitemap methods

* fixed debug sitename

* add error when no sites

* removed commented code

* refactored transformLanguageSitePaths

* Update packages/sitecore-jss-nextjs/src/services/graphql-sitemap-service.ts

Co-authored-by: Adam Brauer <400763+ambrauer@users.noreply.github.com>

* lint fix

Co-authored-by: illiakovalenko <illia.kovalenko@sitecore.com>
Co-authored-by: Illia Kovalenko <23364749+illiakovalenko@users.noreply.github.com>
Co-authored-by: Artem Alexeyenko <ala@sitecore.net>
Co-authored-by: Addy Pathania <89087450+sc-addypathania@users.noreply.github.com>
Co-authored-by: Automated Build <builds@sitecore.com>
Co-authored-by: Ruslan Matkovskyi <rusm@sitecore.net>
Co-authored-by: Ruslan Matkovskyi <100142572+matkovskyi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@art-alexeyenko art-alexeyenko art-alexeyenko requested changes

@ambrauer ambrauer ambrauer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@illiakovalenko @art-alexeyenko @ambrauer