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.

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

feat!: remove globalMutableWebpackConfig #439

Merged
merged 1 commit into from
Mar 9, 2024
Merged

feat!: remove globalMutableWebpackConfig #439

merged 1 commit into from
Mar 9, 2024

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Mar 8, 2024

Summary

This was deprecated in v7 in favor of generateWebpackConfig, so now we can remove it

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

@tomdracz tomdracz merged commit 8585bf8 into shakacode:next Mar 9, 2024
43 checks passed
@G-Rath G-Rath deleted the remove-global-mutable-config branch March 9, 2024 17:50
@ahangarha
Copy link
Contributor

@G-Rath
@tomdracz
May I know the background of this PR? Was is discussed anywhere?

@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 9, 2024

@ahangarha I thought it was always intended to remove this and that its use is discouraged - it wasn't as strongly worded as other deprecations, but unless you think there's a solid usecase where generateWebpackConfig can't be used I'd say we stick with removing it because it's not a breaking change to add it back later if it does turn out to be worth keeping and mutating globals is generally considered a very risky thing

@ahangarha
Copy link
Contributor

When we made this change last year, we had a long discussion on how and why to implement it this way. I have no objection for its removal. Just I think Justin should know about this change. If he was not involved in the process, let's share it with him too.

@tomdracz
Copy link
Collaborator

tomdracz commented Mar 9, 2024

Plus 1 to @G-Rath comment here, I've assumed the deprecation was the desire here. I can't see a clear use case for the object being required but happy to be convinced!

When we made this change last year, we had a long discussion on how and why to implement it this way.

@ahangarha was this discussion anywhere in this repo or elsewhere? Haven't seen anything so that might be some context missing (mis)guided me here to merge in the PR

@ahangarha
Copy link
Contributor

ahangarha commented Mar 10, 2024

was this discussion anywhere in this repo or elsewhere?

No. We had a video call.

Let's just share this with Justin before it become harder to revert (in case...).

ahangarha pushed a commit that referenced this pull request Mar 28, 2024
justin808 added a commit that referenced this pull request May 8, 2024
* 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)
@coderabbitai coderabbitai bot mentioned this pull request May 8, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants