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

Strip CDN from manifest output #473

Merged
merged 7 commits into from
May 17, 2024
Merged

Strip CDN from manifest output #473

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb7d0ad
Strip CDN from manifest output
tomdracz May 10, 2024
ee7e32c
Update changelog
tomdracz May 10, 2024
3d5cc48
Add CDN info to the migration guide
tomdracz May 16, 2024
3e7ff83
Apply suggestions from code review
justin808 May 16, 2024
cf41176
Apply suggestions from code review
justin808 May 16, 2024
8001277
Add mention of auto output option
tomdracz May 16, 2024
3fa49d8
Update docs/v8_upgrade.md
justin808 May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ _next_ branch is for v8 changes
## [Unreleased]
Changes since the last non-beta release.

### 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.
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
This returns to the Webpacker behaviour prior to the aborted Webpacker v6.
This returns to the Webpacker behaviour before the aborted Webpacker v6.


### Fixed

- Fixes incorrect removal of files in the assets:clean task [PR 474](https://github.com/shakacode/shakapacker/pull/474) by [tomdracz](https://github.com/tomdracz).
Expand Down
26 changes: 26 additions & 0 deletions docs/v8_upgrade.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,32 @@ 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.
justin808 marked this conversation as resolved.
Show resolved Hide resolved

Presence of this host in the output could lead to unexpected issues and required [some workarounds](https://github.com/shakacode/shakapacker/blob/main/docs/troubleshooting.md#wrong-cdn-src-from-javascript_pack_tag) in certain cases.

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.
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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.
Comment on lines +20 to +22
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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.


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`.
Copy link

Choose a reason for hiding this comment

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

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.


To get around that, you can use dynamic override as outlined by [Webpack documentation](https://webpack.js.org/guides/asset-modules/#on-the-fly-override).

Setting for example:

```
__webpack_public_path__ = 'https://mycdn.url.com/packs';
```
Comment on lines +32 to +34
Copy link

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


## The `packageManager` property in `package.json` is used to determine the package manager

The biggest functional change in v8, `shakapacker` is now able to work with any
justin808 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 1 addition & 1 deletion package/environments/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const getPlugins = () => {
writeToDisk: true,
output: config.manifestPath,
entrypointsUseAssets: true,
publicPath: true
publicPath: config.publicPathWithoutCDN
Copy link
Member

Choose a reason for hiding this comment

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

We need to document who this affects and the migration instructions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool, I'll get something added to the migration guide

Copy link
Member

Choose a reason for hiding this comment

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

Done?

})
]

Expand Down
7 changes: 7 additions & 0 deletions test/package/config.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ describe("Config", () => {
expect(config.publicPath).toBe("http://foo.com/packs/")
})

test("public path without CDN is not affected by the asset host", () => {
process.env.RAILS_ENV = "development"
process.env.SHAKAPACKER_ASSET_HOST = "http://foo.com/"
const config = require("../../package/config")
expect(config.publicPathWithoutCDN).toBe("/packs/")
})

test("should return additional paths as listed in app config, with resolved paths", () => {
const config = require("../../package/config")

Expand Down
Loading