-
Notifications
You must be signed in to change notification settings - Fork 279
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
[templates] Introduce layout service REST configuration name environment variable #1543
Conversation
This variable holds the name of the configuration name that should be used in REST requests to layout service
…fig-generator to expose it in the `config` object
…RestLayoutService`
…into bugfix/578343
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.
In general, looks great (just a couple comments). But looking through this, I wonder if we should make this a purely JSS config-based solution (and lose the .env values). The original PR depended on the .env values to supply the default value, but in this PR we have the fallback/default provided by the JSS config logic.
For nextjs-sxa
, this would mean instead of a .env
we could use a config plugin (e.g. /scripts/config/plugins/sxa.ts
).
Considerations for removing the .env value:
- Environment variables should be something that would change per environment or deployment (yes, we have at least one offender of this rule already :)). I can't imagine a case where this one would.
- Not introducing another .env value (reducing the amount we already have is an internal goal)
- The nice thing about our JSS config architecture is that an environment variable can be used if absolutely necessary
If we do decide to go this route (and remove the .env vals), I think the .env concat > merge logic is a great improvement and should stay either way.
packages/create-sitecore-jss/src/common/processes/transform.test.ts
Outdated
Show resolved
Hide resolved
It's a great proposal! Right, for the future we can introduce more config based variables if we will need it (env vars not required to be added) |
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.
👍
Description / Motivation
Testing Details
Types of changes