Strip CDN from manifest output #473
Conversation
WalkthroughThe changes primarily involve reverting the behavior of Webpacker to exclude CDN URLs in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- package/environments/base.js (1 hunks)
- test/package/config.test.js (1 hunks)
Additional comments not posted (2)
test/package/config.test.js (1)
24-29: The new test case effectively verifies that the public path configuration remains unaffected by the asset host settings, aligning with the PR's objectives.package/environments/base.js (1)
83-83: The modification to setpublicPathtoconfig.publicPathWithoutCDNin theWebpackAssetsManifestplugin aligns with the PR's objectives to handle public paths correctly in the absence of a CDN.
tomdracz
force-pushed
the
strip-cdn-from-manifest-output
branch
4 times, most recently
from
6e295ad to
9b13125
Compare
| @@ -80,7 +80,7 @@ const getPlugins = () => { | |||
| writeToDisk: true, | |||
| output: config.manifestPath, | |||
| entrypointsUseAssets: true, | |||
| publicPath: true | |||
| publicPath: config.publicPathWithoutCDN | |||
There was a problem hiding this comment.
We need to document who this affects and the migration instructions.
There was a problem hiding this comment.
Cool, I'll get something added to the migration guide
tomdracz
force-pushed
the
strip-cdn-from-manifest-output
branch
from
9b13125 to
ee7e32c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- package/environments/base.js (1 hunks)
- test/package/config.test.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- package/environments/base.js
- test/package/config.test.js
Additional Context Used
LanguageTool (49)
CHANGELOG.md (49)
Near line 1: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: * For the changelog of versions prior to v6, see the [5.x stable branch of rails...
Near line 4: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...grading to new spelling in version 7. * Please see the [v6 Upgrade Guide](./docs/v6_up...
Near line 4: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...docs/v6_upgrade.md) to go from versions prior to v6. * [ShakaCode](https://www.shakacode...
Near line 16: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...This returns to the Webpacker behaviour prior to the aborted Webpacker v6. ### Fixed -...
Near line 50: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...G-Rath. - Removehttpsoption forwebpack-dev-server...
Near line 52: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...G-Rath. - Removeverify_file_existancemethod [PR 446]...
Near line 60: Possible missing comma found.
Context: ...additional_pathswill have their path stripped just like with thesource_path. [PR 4...
Near line 69: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ad of errors when compilation is success but stderr is not empty. [PR 416](https://g...
Near line 72: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...G-Rath](https://github.com/g-rath) ### Removed - Removes dependency ongloblibrary. [PR 435](...
Near line 115: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...acker/pull/363). ## [v7.0.3] - July 7, 2023 ### Fixed - Fixed commands execution fo...
Near line 116: Possible typo: you repeated a word
Context: ...l/363). ## [v7.0.3] - July 7, 2023 ### Fixed - Fixed commands execution for projects with sp...
Near line 119: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...b.com/kukicola). ## [v7.0.2] - July 3, 2023 ### Fixed - Fixed creation of assets:pr...
Near line 120: Possible typo: you repeated a word
Context: ...icola). ## [v7.0.2] - July 3, 2023 ### Fixed - Fixed creation of assets:precompile if it is ...
Near line 123: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...com/ahangarha). ## [v7.0.1] - June 27, 2023 ### Fixed - Fixed the condition for sho...
Near line 124: Possible typo: you repeated a word
Context: ...arha). ## [v7.0.1] - June 27, 2023 ### Fixed - Fixed the condition for showing warning for s...
Near line 127: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...com/ahangarha). ## [v7.0.0] - June 23, 2023 ### Breaking changes - Removes defaults...
Near line 131: Possible missing comma found.
Context: ...ery file in the codebase was treated as TSX leading to potential issues. This has b...
Near line 131: Possible missing comma found.
Context: ...is change might affect generated output however so it is marked as breaking. - Export ...
Near line 155: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...dracz](https://github.com/tomdracz). Prior to this change, top level directory of sta...
Near line 155: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...com/tomdracz). Prior to this change, top level directory of static assets like images ...
Near line 155: Possible missing article found.
Context: ...and fonts was stripped. This meant that file inapp/javascript/images/image.pngwo...
Near line 157: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...tic/image.jpg")`. Going forward, the top level directory of static files will be retai...
Near line 157: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...rectory of static files will be retained so this will necessitate the update of fil...
Near line 164: Possible missing comma found.
Context: ...ery file in the codebase was treated as TSX leading to potential issues. This has b...
Near line 164: Possible missing comma found.
Context: ...is change might affect generated output however so it is marked as breaking. - Fixed R...
Near line 168: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...arha](https://github.com/ahangarha) ### Removed - Remove redundant enhancement for precompile ta...
Near line 172: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...com/ahangarha). ## [v6.6.0] - March 7, 2023 ### Improved - Allow configuration of w...
Near line 176: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...alecslupu). ## [v6.5.6] - February 11, 2023 ### Fixed - Fixed failing to update `bi...
Near line 177: Possible typo: you repeated a word
Context: ...). ## [v6.5.6] - February 11, 2023 ### Fixed - Fixed failing to updatebin/setupfile due ...
Near line 195: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...m/tagliala). ## [v6.5.4] - November 4, 2022 ### Fixed - Fixed regression caused by ...
Near line 196: Possible typo: you repeated a word
Context: ...a). ## [v6.5.4] - November 4, 2022 ### Fixed - Fixed regression caused by 6.5.3. PR #192 int...
Near line 229: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ....com/pulkitkkr). ## [v6.5.0] - July 4, 2022 ### Added - `append_stylesheet_pack_tag...
Near line 236: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...ub.com/artemave) ## [v6.4.1] - June 5, 2022 ### Fixed - Restores automatic installa...
Near line 244: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...b.com/tomdracz). ## [v6.4.0] - June 2, 2022 ### Fixed - Fixed [Issue 123: Rails 7.0...
Near line 245: Possible typo: you repeated a word
Context: ...dracz). ## [v6.4.0] - June 2, 2022 ### Fixed - Fixed [Issue 123: Rails 7.0.3 - Webpacker con...
Near line 249: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...se nested entries. This was the default prior to v6.0. Because entries maybe generated, ...
Near line 254: Possible missing comma found.
Context: ...urself. Should you wish to restore this behaviour you'll need to [reimplement the task](h...
Near line 254: If this is a question, use a question mark.
Context: ...tasks/yarn.rake) in your own application. ## [v6.3.0] - May 19, 2022 ### Improv...
Near line 262: Possible missing comma found.
Context: ...//github.com/tomdracz). ### Fixed - On Windows CSS urls no longer contain backslashes ...
Near line 271: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...e mode can affect how your code runs in a variety of ways, but in newer versions of Babel it...
Near line 275: You’ve repeated a verb. Did you mean to only write one of them?
Context: ... change probably won't affect you. ### Added - Addswebpacker_precompilesetting to `webp...
Near line 282: The modal verb ‘will’ requires the verb’s base form.
Context: ...timestamp will be checked together will timestamps of all files and directories inside of ...
Near line 289: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...com/tomdracz). ## [v6.2.1] - April 15, 2022 ### Fixed - Put back config.public_mani...
Near line 303: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...ple invocations of stylesheet_pack_tag (eg for a regular stylesheet & a print styl...
Near line 311: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...m/tomdracz). ## [v6.1.0] - February 4, 2022 ### Added - Support for SWC loader. [PR...
Near line 318: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: .../pull/2802). ## [v6.0.2] - January 25, 2022 ### Improved - Fix incorrect command na...
Near line 322: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...tricknotes). ## [v6.0.1] - January 24, 2022 ### Improved - PR #21 removed pnp-webpa...
Near line 355: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...renvironment.js, this must be removed or you will receive an error (`Item nodeMo...
Near line 356: The preposition ‘to’ seems more likely in this position.
Context: ...iningCss. This is useful to enable HMR for React. - Webpacker's wrapper to thesp...
Additional comments not posted (2)
CHANGELOG.md (2)
13-14: The breaking change related to the removal of CDN URLs from the manifest.json paths is well-documented and references the relevant PR and contributor. The description is clear and concise.
17-17: The fix for the incorrect removal of files in the assets:clean task is well-documented and references the relevant PR and contributor. The description is clear and concise.
| ### Breaking changes | ||
| - Removes CDN url from the manifest.json paths. [PR 473](https://github.com/shakacode/shakapacker/pull/473) by [tomdracz](https://github.com/tomdracz). | ||
|
|
||
| This returns to the Webpacker behaviour prior to the aborted Webpacker v6. |
There was a problem hiding this comment.
Consider simplifying "prior to" to "before" for clarity.
- This returns to the Webpacker behaviour prior to the aborted Webpacker v6.
+ This returns to the Webpacker behaviour before the aborted Webpacker v6.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| This returns to the Webpacker behaviour prior to the aborted Webpacker v6. | |
| This returns to the Webpacker behaviour before the aborted Webpacker v6. |
There was a problem hiding this comment.
Actionable comments posted: 6
Out of diff range and nitpick comments (3)
docs/v8_upgrade.md (3)
Line range hint
51-51: Add the missing verb.- but it important the property is set to ensure all tooling uses the right package manager. + but it is important the property is set to ensure all tooling uses the right package manager.
Line range hint
73-73: Add a comma for clarity.- will install dependencies automatically but if you're using a tool like `capistrano` + will install dependencies automatically, but if you're using a tool like `capistrano`
Line range hint
155-155: Add the missing article.- going forward asset paths should be same regardless of their source: + going forward, asset paths should be the same regardless of their source:
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- docs/v8_upgrade.md (1 hunks)
Additional Context Used
LanguageTool (8)
docs/v8_upgrade.md (8)
Near line 7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...v12 has also been dropped since they're very old at this point. ## CDN host is stripped...
Near line 12: Possible missing comma found.
Context: ... from the manifest output In Webpacker v5 the manifest.json file did not include ...
Near line 12: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...d. THis has been added in the aborted v6 and we've retained this in Shakapacker. Pr...
Near line 19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ferencing assets through view helpers. If your host might differ, between various...
Near line 38: As a shorter alternative for ‘able to’, consider using “can now”.
Context: ... functional change in v8,shakapackeris now able to work with any of the major JavaScript p...
Near line 51: A verb may be missing after “it”. Please verify.
Context: ... with the recommended value to use, but it important the property is set to ensure all tooli...
Near line 73: Possible missing comma found.
Context: ... will install dependencies automatically but if you're using a tool like `capistrano...
Near line 155: Possible missing article found.
Context: ...ans going forward asset paths should be same regardless of their source: ```erb <%#...
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- docs/v8_upgrade.md (1 hunks)
Files not reviewed due to errors (1)
- docs/v8_upgrade.md (no review received)
Additional Context Used
LanguageTool (7)
docs/v8_upgrade.md (7)
Near line 7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...v12 has also been dropped since they're very old at this point. ## CDN host is stripped...
Near line 12: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...d. THis has been added in the aborted v6 and we've retained this in Shakapacker. Pr...
Near line 19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ferencing assets through view helpers. If your host might differ, between various...
Near line 23: Possible missing article found.
Context: ...void hardcording URLs in packs output. Second option has got a certain gotcha - dynam...
Near line 38: As a shorter alternative for ‘able to’, consider using “can now”.
Context: ... functional change in v8,shakapackeris now able to work with any of the major JavaScript p...
Near line 51: A verb may be missing after “it”. Please verify.
Context: ... with the recommended value to use, but it important the property is set to ensure all tooli...
Near line 106: Possible missing preposition found.
Context: ...bled by default This hasshakapackercheck that the versions of the installed Ruby...
| __webpack_public_path__ = 'https://mycdn.url.com/packs'; | ||
| ``` | ||
|
|
||
| In your code and ensuring it is run first in the app, will allow the dynamic imports lookup path to be overriden at runtime. |
There was a problem hiding this comment.
Overall, the setting of an assets CDN during compilation is maybe a huge anti-pattern as it precludes using the same bundles on staging and production unless your assets are exactly the same and properly fingerprinted.
Thoughts? Should this be mentioned?
In other words, shouldn't the CDN be set via an ENV value and not put into the builds?
There was a problem hiding this comment.
Overall, the setting of an assets CDN during compilation is maybe a huge anti-pattern as it precludes using the same bundles on staging and production unless your assets are exactly the same and properly fingerprinted.
It's kinda what Webpack does though and usually mentions in their docs when CDN comes into play https://webpack.js.org/guides/public-path/#environment-based
In other words, shouldn't the CDN be set via an ENV value and not put into the builds?
__webpack_public_path__ will let you do something like that but there's extra steps involved - how do you pass that ENV value to the build output?
You could probably do something like __webpack_public_path__ = window.publicPathOverride but then ensure that this is executed and available in your app view code
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- docs/v8_upgrade.md (1 hunks)
Files not reviewed due to errors (1)
- docs/v8_upgrade.md (no review received)
Additional Context Used
LanguageTool (7)
docs/v8_upgrade.md (7)
Near line 7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...v12 has also been dropped since they're very old at this point. ## CDN host is stripped...
Near line 12: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...d. THis has been added in the aborted v6 and we've retained this in Shakapacker. Pr...
Near line 19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ferencing assets through view helpers. If your host might differ, between various...
Near line 23: Possible missing article found.
Context: ...void hardcording URLs in packs output. Second option has got a certain gotcha - dynam...
Near line 40: As a shorter alternative for ‘able to’, consider using “can now”.
Context: ... functional change in v8,shakapackeris now able to work with any of the major JavaScript p...
Near line 53: A verb may be missing after “it”. Please verify.
Context: ... with the recommended value to use, but it important the property is set to ensure all tooli...
Near line 157: Possible missing article found.
Context: ...ans going forward asset paths should be same regardless of their source: ```erb <%#...
| You can also try Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath. | ||
|
|
||
| For example in your `webpack.config.js`: | ||
|
|
||
| ``` | ||
| const { generateWebpackConfig } = require('shakapacker') | ||
|
|
||
| const customConfig = { | ||
| output: { | ||
| publicPath: 'auto' | ||
| } | ||
| }; | ||
|
|
||
| module.exports = generateWebpackConfig(customConfig); | ||
| ``` | ||
|
|
||
| This will work in number of environments although some older browsers like IE will require a polyfill as mentioned in the Webpack documentation linked above. | ||
|
|
There was a problem hiding this comment.
Documented usage of auto publicPath that should be a happy-medium for the CDN assets. With this option and config.asset_host specified I see all imports coming from that host defined @justin808
Might consider making this a default or opt-in behaviour but would need to dig further to understand consequences of that
There was a problem hiding this comment.
Actionable comments posted: 10
Out of diff range and nitpick comments (3)
docs/v8_upgrade.md (3)
Line range hint
71-71: A verb may be missing after "it". Please verify.- with the recommended value to use, but it important the property is set to ensure all tooling uses the right package manager. + with the recommended value to use, but it is important the property is set to ensure all tooling uses the right package manager.
Line range hint
93-93: Add a comma for clarity.- Some platforms like Heroku will install dependencies automatically but if you're using a tool like `capistrano` to deploy to servers you can enhance the `assets:precompile` command like so: + Some platforms like Heroku will install dependencies automatically, but if you're using a tool like `capistrano` to deploy to servers, you can enhance the `assets:precompile` command like so:
Line range hint
175-175: Add the missing article "the".- This means going forward asset paths should be same regardless of their source: + This means going forward the asset paths should be the same regardless of their source:
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- docs/v8_upgrade.md (1 hunks)
Additional Context Used
LanguageTool (15)
docs/v8_upgrade.md (15)
Near line 7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...v12 has also been dropped since they're very old at this point. ## CDN host is stripped...
Near line 12: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...d. THis has been added in the aborted v6 and we've retained this in Shakapacker. Pr...
Near line 18: You might be missing the article “the” here.
Context: ... using CDN and your CDN host is static,config.asset_hostsetting in Rails will...
Near line 19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ferencing assets through view helpers. If your host might differ, between various...
Near line 22: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...V variable to avoid hardcording URLs in packs output. Second option has got a certai...
Near line 23: A determiner appears to be missing. Consider inserting it.
Context: ...void hardcording URLs in packs output. Second option has got a certain gotcha - dynam...
Near line 26: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...NV variable to avoid hardcoding URLs in packs output. To get around that, you can us...
Near line 38: You might be missing the article “the” here.
Context: ...overriden at runtime. You can also try Webpackoutput.publicPathoption of'auto'...
Near line 40: After the expression ‘for example’ a comma is usually used.
Context: ...public-path/#automatic-publicpath. For example in yourwebpack.config.js: ``` const...
Near line 54: You might be missing the article “a” here.
Context: ...g(customConfig); ``` This will work in number of environments although some older bro...
Near line 54: Use a comma before ‘although’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...This will work in number of environments although some older browsers like IE will requir...
Near line 58: As a shorter alternative for ‘able to’, consider using “can now”.
Context: ... functional change in v8,shakapackeris now able to work with any of the major JavaScript p...
Near line 71: A verb may be missing after “it”. Please verify.
Context: ... with the recommended value to use, but it important the property is set to ensure all tooli...
Near line 93: A comma might be missing here.
Context: ...s like Heroku will install dependencies automatically but if you're using a tool like `capist...
Near line 175: Possible missing article found.
Context: ...ans going forward asset paths should be same regardless of their source: ```erb <%#...
| @@ -7,6 +7,52 @@ manager is used to manage JavaScript dependencies. | |||
| Support for Ruby 2.6 and Node v12 has also been dropped since they're very old | |||
| at this point. | |||
|
|
|||
| ## CDN host is stripped from the manifest output | |||
|
|
|||
| In Webpacker v5, the manifest.json file did not include the CDN asset host if defined. THis has been added in the aborted v6 and we've retained this in Shakapacker. | |||
There was a problem hiding this comment.
Correct the capitalization and add a comma.
- THis has been added in the aborted v6 and we've retained this in Shakapacker.
+ This has been added in the aborted v6, and we've retained this in Shakapacker.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| In Webpacker v5, the manifest.json file did not include the CDN asset host if defined. THis has been added in the aborted v6 and we've retained this in Shakapacker. | |
| In Webpacker v5, the manifest.json file did not include the CDN asset host if defined. This has been added in the aborted v6, and we've retained this in Shakapacker. |
|
|
||
| If you are not using CDN, then this change will have no effect on your setup. | ||
|
|
||
| If you are using CDN and your CDN host is static, `config.asset_host` setting in Rails will be respected during compilation and when referencing assets through view helpers. |
There was a problem hiding this comment.
Add the missing article "the".
- If you are using CDN and your CDN host is static, `config.asset_host` setting in Rails will be respected during compilation and when referencing assets through view helpers.
+ If you are using CDN and your CDN host is static, the `config.asset_host` setting in Rails will be respected during compilation and when referencing assets through view helpers.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| If you are using CDN and your CDN host is static, `config.asset_host` setting in Rails will be respected during compilation and when referencing assets through view helpers. | |
| If you are using CDN and your CDN host is static, the `config.asset_host` setting in Rails will be respected during compilation and when referencing assets through view helpers. |
| If your host might differ, between various environments for example, you will either need to: | ||
| - Ensure the assets are specifically rebuilt for each environment (Heroku pipeline promote feature for example does not do that by default). | ||
| - Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcording URLs in packs output. |
There was a problem hiding this comment.
Ensure lists are surrounded by blank lines.
- If your host might differ, between various environments for example, you will either need to:
- - Ensure the assets are specifically rebuilt for each environment (Heroku pipeline promote feature for example does not do that by default).
- - Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcording URLs in packs output.
+ If your host might differ, between various environments for example, you will either need to:
+ - Ensure the assets are specifically rebuilt for each environment (Heroku pipeline promote feature for example does not do that by default).
+ - Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcoding URLs in packs output.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| If your host might differ, between various environments for example, you will either need to: | |
| - Ensure the assets are specifically rebuilt for each environment (Heroku pipeline promote feature for example does not do that by default). | |
| - Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcording URLs in packs output. | |
| If your host might differ, between various environments for example, you will either need to: | |
| - Ensure the assets are specifically rebuilt for each environment (Heroku pipeline promote feature for example does not do that by default). | |
| - Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcoding URLs in packs output. |
| - Ensure the assets are specifically rebuilt for each environment (Heroku pipeline promote feature for example does not do that by default). | ||
| - Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcording URLs in packs output. | ||
|
|
||
| Second option has got a certain gotcha - dynamic imports and static asset references (like image paths in CSS) will end up without host reference and the app will try and fetch them from your app host rather than defined `config.asset_host`. |
There was a problem hiding this comment.
Correct the spelling of "hardcoding".
- Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcording URLs in packs output.
+ Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcoding URLs in packs output.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Second option has got a certain gotcha - dynamic imports and static asset references (like image paths in CSS) will end up without host reference and the app will try and fetch them from your app host rather than defined `config.asset_host`.
Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcoding URLs in packs output.
| ``` | ||
| __webpack_public_path__ = 'https://mycdn.url.com/packs'; | ||
| ``` |
There was a problem hiding this comment.
Specify the language for the code block.
- ```
+ ```javascript|
|
||
| In your code and ensuring it is run first in the app, will allow the dynamic imports lookup path to be overriden at runtime. | ||
|
|
||
| You can also try Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath. |
There was a problem hiding this comment.
Add the missing article "the".
- You can also try Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath.
+ You can also try the Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| You can also try Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath. | |
| You can also try the Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath. |
|
|
||
| You can also try Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath. | ||
|
|
||
| For example in your `webpack.config.js`: |
There was a problem hiding this comment.
Add a comma after "for example".
- For example in your `webpack.config.js`:
+ For example, in your `webpack.config.js`:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| For example in your `webpack.config.js`: | |
| For example, in your `webpack.config.js`: |
| ``` | ||
| const { generateWebpackConfig } = require('shakapacker') | ||
|
|
||
| const customConfig = { | ||
| output: { | ||
| publicPath: 'auto' | ||
| } | ||
| }; | ||
|
|
||
| module.exports = generateWebpackConfig(customConfig); | ||
| ``` |
There was a problem hiding this comment.
Specify the language for the code block.
- ```
+ ```javascript| module.exports = generateWebpackConfig(customConfig); | ||
| ``` | ||
|
|
||
| This will work in number of environments although some older browsers like IE will require a polyfill as mentioned in the Webpack documentation linked above. |
There was a problem hiding this comment.
Add the missing article "a".
- This will work in number of environments although some older browsers like IE will require a polyfill as mentioned in the Webpack documentation linked above.
+ This will work in a number of environments although some older browsers like IE will require a polyfill as mentioned in the Webpack documentation linked above.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| This will work in number of environments although some older browsers like IE will require a polyfill as mentioned in the Webpack documentation linked above. | |
| This will work in a number of environments although some older browsers like IE will require a polyfill as mentioned in the Webpack documentation linked above. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- docs/v8_upgrade.md (1 hunks)
Additional Context Used
LanguageTool (9)
docs/v8_upgrade.md (9)
Near line 7: Possible missing comma found.
Context: ...for Ruby 2.6 and Node v12 has also been dropped since they're very old at this point. ...
Near line 7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...v12 has also been dropped since they're very old at this point. ## CDN host is stripped...
Near line 12: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...d. THis has been added in the aborted v6 and we've retained this in Shakapacker. Pr...
Near line 19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ferencing assets through view helpers. If your host might differ, between various...
Near line 23: Possible missing article found.
Context: ...void hardcording URLs in packs output. Second option has got a certain gotcha - dynam...
Near line 40: After the expression ‘for example’ a comma is usually used.
Context: ...public-path/#automatic-publicpath. For example in yourwebpack.config.js: ``` const...
Near line 54: Possible missing article found.
Context: ...g(customConfig); ``` This will work in number of environments although some older bro...
Near line 54: Use a comma before ‘although’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...This will work in number of environments although some older browsers like IE will requir...
Near line 71: A verb may be missing after “it”. Please verify.
Context: ... with the recommended value to use, but it important the property is set to ensure all tooli...
Additional comments not posted (9)
docs/v8_upgrade.md (9)
12-12: Correct the capitalization and add a comma.- THis has been added in the aborted v6 and we've retained this in Shakapacker. + This has been added in the aborted v6, and we've retained this in Shakapacker.
18-18: Add the missing article "the".- If you are using CDN and your CDN host is static, `config.asset_host` setting in Rails will be respected during compilation and when referencing assets through view helpers. + If you are using CDN and your CDN host is static, the `config.asset_host` setting in Rails will be respected during compilation and when referencing assets through view helpers.
20-22: Ensure lists are surrounded by blank lines.- If your host might differ, between various environments for example, you will either need to: - - Ensure the assets are specifically rebuilt for each environment (Heroku pipeline promote feature for example does not do that by default). - - Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcording URLs in packs output. + If your host might differ, between various environments for example, you will either need to: + - Ensure the assets are specifically rebuilt for each environment (Heroku pipeline promote feature for example does not do that by default). + - Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcoding URLs in packs output.
24-24: Correct the spelling of "hardcoding".- Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcording URLs in packs output. + Make sure the assets are compiled with `SHAKAPACKER_ASSET_HOST=''` ENV variable to avoid hardcoding URLs in packs output.
32-34: Specify the language for the code block.- ``` + ```javascript
38-38: Add the missing article "the".- You can also try Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath. + You can also try the Webpack `output.publicPath` option of `'auto'` as per https://webpack.js.org/guides/public-path/#automatic-publicpath.
40-40: Add a comma after "for example".- For example in your `webpack.config.js`: + For example, in your `webpack.config.js`:
42-52: Specify the language for the code block.- ``` + ```javascript
54-54: Add the missing article "a".- This will work in number of environments although some older browsers like IE will require a polyfill as mentioned in the Webpack documentation linked above. + This will work in a number of environments although some older browsers like IE will require a polyfill as mentioned in the Webpack documentation linked above.
Summary
Fixes #151
Asset manifests path used to be output without CDN as per rails/webpacker#1974
But then aborted Webpacker v6 added the CDN back for reasons unknown rails/webpacker#2802
This is causing issues with the clean tasks and requires other workarounds as outlined in https://github.com/shakacode/shakapacker/blob/main/docs/troubleshooting.md#wrong-cdn-src-from-javascript_pack_tag
The PR reverts this behaviour so the assets are once again listed in the manifest output without CDN
Pull Request checklist
Summary by CodeRabbit
Bug Fixes
Documentation
Tests