Pass in shakapacker config consistently #448
Conversation
| @@ -106,7 +106,7 @@ def webpack_env | |||
|
|
|||
| env.merge( | |||
| "SHAKAPACKER_ASSET_HOST" => instance.config.asset_host, | |||
| "SHAKAPACKER_CONFIG" => instance.config_path.to_s | |||
| "SHAKAPACKER_CONFIG" => instance.config.config_path.to_s | |||
There was a problem hiding this comment.
Technically could be left as is since both places should have same source of truth but makes it so we're consistent with the line above
| @config_path = Pathname.new(ENV["SHAKAPACKER_CONFIG"] || config_path) | ||
| @config_path = config_path |
There was a problem hiding this comment.
Isn't it better to use our configuration as the single source of truth? Only the Configuration class should care about how to set such variables and others should only and only refer to the Configuration instance to know the value.
I remember once we discussed implementing this gradually. At the moment, I don't remember why I checked SHAKAPACKER_CONFIG both in the Configuration and in the Instance classes. For this, I need to review the changes and possibly chats from that time.
There was a problem hiding this comment.
I see that the change in the Instance class is new. In the past, we only handled this issue in the Configuration class.
There was a problem hiding this comment.
I'm happy with this being wherever, just needs to be consistent.
In runner
shakapacker/lib/shakapacker/runner.rb
Line 18 in 3be8b49
In Env we were checking it only on the instance
shakapacker/lib/shakapacker/env.rb
Line 2 in 49e4b2c
If we want to keep on the instance, then need to address those two. Happy to do that if that's what we want to do instead of the instance
There was a problem hiding this comment.
You are right.
I think the right thing is that anything related to configuration to be handled by and fetched from the Configuration class.
There was a problem hiding this comment.
Amended @ahangarha Need to dig through spec failures though
There was a problem hiding this comment.
Actually there's fun circular dependency now:
- instance tries to get env
- env is needed to create config
- kaboom!
IMO this
shakapacker/lib/shakapacker/env.rb
Line 27 in 49e4b2c
It's either a deeper refactor needed or back to what it was and doing the change on Instance level since that's the common ancestor. Thoughts? Reverted the changes back to moving config set on Instance for now
| @@ -7,7 +7,7 @@ class Shakapacker::Instance | |||
|
|
|||
| def initialize(root_path: Rails.root, config_path: Rails.root.join("config/shakapacker.yml")) | |||
| @root_path = root_path | |||
| @config_path = config_path | |||
| @config_path = Pathname.new(ENV["SHAKAPACKER_CONFIG"] || config_path) | |||
There was a problem hiding this comment.
As per https://github.com/shakacode/shakapacker/pull/448/files#r1519695275
We check for available environments in lib/shakapacker/env.rb - that was disregarding custom files, so for example if my config passed through ENV defined a new environment, it would be not picked up and fallback on production env
Since Configuration needs env instance passed by itself, it cannot just look at the instance.config as it will create circular dependency
There was a problem hiding this comment.
Hmmm...
Interesting. Then let's see if we need to do some refactoring (perhaps must be done before v8 release)
* Reference config from config rather than instance in compiler
* Remove deprecated webpacker spelling (#429) * feat!: drop support for Ruby 2.6 (#415) * feat!: remove `https` option (#414) * feat!: remove `relative_url_root` option (#413) * feat!: drop support for Node v12 (#431) * feat!: remove `yarn_install` task (#412) * ci: cancel in-progress jobs when new changes are pushed (#432) * ci: merge ruby and node based jobs into single workflows (#434) * ci: merge node related checks into a single workflow * ci: merge ruby related checks into a single workflow * ci: rename jobs * ci: test against Ruby 3.1 and 3.2, and Node 20 (#433) * ci: test against Ruby 3.1 and 3.2 * ci: test against Node 20 * ci: only run Rubocop with latest Ruby (#438) * chore: update pull request template (#437) * ci: test against Rails 7.1 (#436) * feat!: remove `globalMutableWebpackConfig` (#439) * ci: don't run on pushes to `next` (#444) * feat!: remove `verify_file_existance` method (#446) * feat!: remove `check_yarn` task (#443) * ci: use latest bundler in generator jobs (#445) * feat!: enable `ensure_consistent_versioning` by default (#447) * Remove dependency on glob (#435) * Remove dependency on glob * Pass in shakapacker config consistently (#448) * Reference config from config rather than instance in compiler * Strip additional_paths from the asset paths generated in the file.js rule (#403) * Add config.includePaths and strip includePaths from file.js output paths * Refactor code to fix lint issue * Make sure to replace only once * Change variable name * Revert includePath change and make stripping additional_paths opt-in * refactor: use snake_case for method name (#450) * fix: only strip paths that start with source path (#451) * feat!: always use `package_json` for managing node packages (#430) * feat!: minor cleanup of js utilities (#454) * test: require `ostruct` explicitly (#460) * ci: only run ESLint on latest Node (#462) * docs: create upgrade guide for v8 (#458) * V8 upgrade guide * docs: write v8 upgrade doc * docs: format with Prettier * feat!: move tests and helpers into (#459) * test: move into `test` directory * test: update import paths * test: update jest config * refactor: move test helpers * test: move jest config into dedicated file * docs: add changelog entry * chore: remove unneeded eslint ignore * refactor: update js linting (#461) * refactor: setup and apply Prettier (#463) * refactor: use `eslint-plugin-jest` (#464)
Summary
Whilst investigating #408 I couldn't quite replicate the issues but did notice there a few places in the code where we were not respecting
SHAKAPACKER_CONFIGenv variable. This moves it up one level to mainShakapacker::Instanceso it should be used consistently.There are places like runners where we were using configuration object directly but they were already accounting for path set by env variable.