-
Notifications
You must be signed in to change notification settings - Fork 60
feat(cdn): allows customization of the CDN base url #422
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
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.
Had to change the package.json because unpkg can't serve 2.8.0 for some reason but it can 3.0.0 so this unbreaks the default case in the configurator
3f8e15a to
cd9d331
Compare
sorvell
left a comment
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.
Thanks so much! optional nits.
|
Test failure seems like a flake |
Allows for setting the CDN base url. Currently only tested is unpkg and https://cdn.jsdelivr.net/npm via the `cdnBaseUrl` property and config entry
| @property({attribute: 'cdn-base-url'}) | ||
| cdnBaseUrl = 'https://unpkg.com'; |
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.
do we need this as a separate property on <playground-project> if it's already part of the config property, or the JSON in projectSrc?
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.
The idea was that the property takes precedence over the config and if neither are defined then it defaults to unpkg.com. But actually what you're pointing out right now as well is that by setting this default on the property, it will by default not read from the config unless you explicitly set ''
I'll work on a lower priority followup to this
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.
#423 but I think your original question is still valid. It's a decision I made to support both. My reasoning was that if we wanted to set this across lit.dev, we'd only have to change the base config that is shared across basically all the projects
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.
yeah keeping it in the JSON manifest for the project-src or config property which share the same interface makes sense, and that'll allow us to update that for the whole of lit.dev playground in relatively few places since we extend from just a few base project jsons.
adding the extra cdnBaseUrl property on top of that to <playground-project> that overrides the project manifest doesn't seem as necessary to me and we'd have to clearly document which value takes precedence if we do keep it.
Allows for setting the CDN base url. Currently only tested is unpkg and https://cdn.jsdelivr.net/npm via the
cdnBaseUrlproperty and config entry