-
Notifications
You must be signed in to change notification settings - Fork 88
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
Made 'originPath' configurable #12
base: master
Are you sure you want to change the base?
Conversation
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.
Hey! First of all thanks for this PR! GJ 👍
It will require quite a bunch of work still tho ⚡️
*/ | ||
|
||
|
||
|
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.
Please, get rid of this meta data :)
if (originPath !== null) { | ||
distributionConfig.Origins[0].OriginPath = originPath; | ||
} else { | ||
delete distributionConfig.Origins[0].OriginPath; |
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.
This else
block will:
- never be called
- will ruin backward compatibility if will be called in any particular way
Please, get rid of it :)
distributionConfig.Origins[0].OriginPath = `/${this.options.stage}`; | ||
const originPath = this.getConfig('originPath', `/${this.options.stage}`); | ||
|
||
if (originPath !== null) { |
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.
This comparison will always be true as getConfig
returns default value in case of originPath
missing in config.
I would suggest to get rid of it :)
@@ -44,6 +44,7 @@ plugins: | |||
custom: | |||
apiCloudFront: | |||
domain: my-custom-domain.com | |||
originPath: /some-path |
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.
Can we get some additional description about this field? :)
I needed to modify
originPath
for my current project and because this value is hardcoded in the current version of the plugin, I've made this option configurable and I hope you can accept this PR asap.