-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
fix: replace Webpacker with Shakapacker #1622
Changes from 70 commits
156df7f
6df4e8d
5051a55
6212747
39b32e2
8c0d019
7723165
797ea16
f1c6fd7
8559b2c
d832db4
4609d03
812c94c
328aacb
52c0ea8
3711e9c
7810da0
0d82c02
2ec90b8
ae8fba8
bfaf244
d33ad0a
446c090
8a0003e
15db710
789958e
96bcb5f
e82d924
6cc3d4f
25edd25
de5bfaf
07f2e72
19ddd8c
5cc7a81
62c69db
4fac40f
4c9568a
32945f7
0f3bdcd
861d544
60b0ec0
e2090b4
1017815
f3be6af
24c8d5a
00827e4
0f6ddcb
15cbe82
b6d0201
196dd26
6ba18dc
d6300a3
8b673f2
2e94227
321adf0
6444653
8162ee7
a694c99
2a5d78d
aaa2140
afe89b3
428fd66
ed3806f
21fa4ea
43d15d0
9b063e7
6a640c5
2726d61
4ca8cf1
a03e171
3a8df96
d77929e
4c3cb9b
1915582
d08a465
2061f3a
82a2aa7
b5e7939
4cce360
c96a47e
b8c5d28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -10,8 +10,7 @@ jobs: | |||||||||||
build-dummy-app-webpack-test-bundles: | ||||||||||||
strategy: | ||||||||||||
matrix: | ||||||||||||
ruby: [3.0, 3.3] | ||||||||||||
node: [16, 20] | ||||||||||||
versions: ['oldest', 'newest'] | ||||||||||||
runs-on: ubuntu-22.04 | ||||||||||||
steps: | ||||||||||||
- uses: actions/checkout@v4 | ||||||||||||
|
@@ -20,7 +19,7 @@ jobs: | |||||||||||
- name: Setup Ruby | ||||||||||||
uses: ruby/setup-ruby@v1 | ||||||||||||
with: | ||||||||||||
ruby-version: ${{ matrix.ruby }} | ||||||||||||
ruby-version: ${{ matrix.versions == 'oldest' && '3.0' || '3.3' }} | ||||||||||||
bundler: 2.5.9 | ||||||||||||
# libyaml-dev is needed for psych v5 | ||||||||||||
# this gem depends on sdoc which depends on rdoc which depends on psych | ||||||||||||
|
@@ -29,7 +28,7 @@ jobs: | |||||||||||
- name: Setup Node | ||||||||||||
uses: actions/setup-node@v3 | ||||||||||||
with: | ||||||||||||
node-version: ${{ matrix.node }} | ||||||||||||
node-version: ${{ matrix.versions == 'oldest' && '16' || '20' }} | ||||||||||||
- name: Print system information | ||||||||||||
run: | | ||||||||||||
echo "Linux release: "; cat /etc/issue | ||||||||||||
|
@@ -55,36 +54,39 @@ jobs: | |||||||||||
uses: actions/cache@v3 | ||||||||||||
with: | ||||||||||||
path: spec/dummy/node_modules | ||||||||||||
key: v5-dummy-app-node-modules-cache-${{ hashFiles('spec/dummy/yarn.lock') }} | ||||||||||||
key: dummy-app-node-modules-cache-${{ hashFiles('spec/dummy/package.json') }}-${{ matrix.versions }} | ||||||||||||
- name: yalc add react-on-rails | ||||||||||||
run: cd spec/dummy && yalc add react-on-rails | ||||||||||||
- name: Install Node modules with Yarn for dummy app | ||||||||||||
run: cd spec/dummy && yarn install --no-progress --no-emoji | ||||||||||||
- run: cd spec/dummy && yarn add "shakapacker@${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}" | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid non-idempotent script execution for adding Shakapacker. Appending directly to the Gemfile in a CI script can lead to non-idempotent behavior if the script runs multiple times. Consider checking if the gem entry already exists before appending. - echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}" >> spec/dummy/Gemfile
+ if ! grep -q "gem 'shakapacker'" spec/dummy/Gemfile; then
+ echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}" >> spec/dummy/Gemfile
+ fi Also applies to: 147-147 |
||||||||||||
- name: Save dummy app ruby gems to cache | ||||||||||||
uses: actions/cache@v3 | ||||||||||||
with: | ||||||||||||
path: spec/dummy/vendor/bundle | ||||||||||||
key: v5-dummy-app-gem-cache-${{ hashFiles('spec/dummy/Gemfile.lock') }} | ||||||||||||
key: dummy-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | ||||||||||||
- name: Dynamically add the right shakapacker version to the Gemfile | ||||||||||||
run: echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> spec/dummy/Gemfile | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure dynamic Gemfile updates are idempotent. Appending directly to the Gemfile in a CI script can lead to non-idempotent behavior if the script runs multiple times. Consider checking if the gem entry already exists before appending. - echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> Gemfile
+ if ! grep -q "gem 'shakapacker'" Gemfile; then
+ echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}" >> Gemfile
+ fi Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's have the latest version back in spec/dummy and have the CI script modify the gemfile to test the older version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @justin808, I will adjust the CI script to ensure that the - name: Set default Shakapacker version in Gemfile
run: |
if ! grep -q "gem 'shakapacker'" spec/dummy/Gemfile; then
echo "gem 'shakapacker', '~> 8.0.0'" >> spec/dummy/Gemfile
fi
- name: Conditionally use older Shakapacker version for testing
if: matrix.versions == 'oldest'
run: |
sed -i "s/gem 'shakapacker', '~> 8.0.0'/gem 'shakapacker', '~> 6.6.0'/" spec/dummy/Gemfile This setup ensures that the Gemfile initially sets the latest version of Shakapacker ( |
||||||||||||
- name: Install Ruby Gems for dummy app | ||||||||||||
run: cd spec/dummy && bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 | ||||||||||||
- name: generate file system-based packs | ||||||||||||
run: cd spec/dummy && RAILS_ENV=test bundle exec rake react_on_rails:generate_packs | ||||||||||||
- name: Build test bundles for dummy app | ||||||||||||
run: cd spec/dummy && yarn run build:test | ||||||||||||
run: cd spec/dummy && rm -rf public/webpack/test && yarn build:rescript && RAILS_ENV=test NODE_ENV=test bin/${{ matrix.versions == 'oldest' && 'web' || 'shaka' }}packer | ||||||||||||
- id: get-sha | ||||||||||||
run: echo "::set-output name=sha::$(git rev-parse HEAD)" | ||||||||||||
- name: Save test webpack bundles to cache (for build number checksum used by rspec job) | ||||||||||||
uses: actions/cache/save@v3 | ||||||||||||
with: | ||||||||||||
path: spec/dummy/public/webpack | ||||||||||||
key: v5-dummy-app-webpack-bundle-${{ steps.get-sha.outputs.sha }} | ||||||||||||
key: dummy-app-webpack-bundle-${{ steps.get-sha.outputs.sha }}-${{ matrix.versions }} | ||||||||||||
|
||||||||||||
main: | ||||||||||||
dummy-app-integration-tests: | ||||||||||||
needs: build-dummy-app-webpack-test-bundles | ||||||||||||
strategy: | ||||||||||||
fail-fast: false | ||||||||||||
matrix: | ||||||||||||
ruby: [3.0, 3.3] | ||||||||||||
node: [16, 20] | ||||||||||||
versions: ['oldest', 'newest'] | ||||||||||||
runs-on: ubuntu-22.04 | ||||||||||||
steps: | ||||||||||||
- uses: actions/checkout@v4 | ||||||||||||
|
@@ -93,12 +95,12 @@ jobs: | |||||||||||
- name: Setup Ruby | ||||||||||||
uses: ruby/setup-ruby@v1 | ||||||||||||
with: | ||||||||||||
ruby-version: ${{ matrix.ruby }} | ||||||||||||
ruby-version: ${{ matrix.versions == 'oldest' && '3.0' || '3.3' }} | ||||||||||||
bundler: 2.5.9 | ||||||||||||
- name: Setup Node | ||||||||||||
uses: actions/setup-node@v3 | ||||||||||||
with: | ||||||||||||
node-version: ${{ matrix.node }} | ||||||||||||
node-version: ${{ matrix.versions == 'oldest' && '16' || '20' }} | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 22 not 20 |
||||||||||||
- name: Print system information | ||||||||||||
run: | | ||||||||||||
echo "Linux release: "; cat /etc/issue | ||||||||||||
|
@@ -117,33 +119,37 @@ jobs: | |||||||||||
uses: actions/cache@v3 | ||||||||||||
with: | ||||||||||||
path: vendor/bundle | ||||||||||||
key: v5-package-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }} | ||||||||||||
key: package-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | ||||||||||||
- name: Save dummy app ruby gems to cache | ||||||||||||
uses: actions/cache@v3 | ||||||||||||
with: | ||||||||||||
path: spec/dummy/vendor/bundle | ||||||||||||
key: v5-dummy-app-gem-cache-${{ hashFiles('spec/dummy/Gemfile.lock') }} | ||||||||||||
key: dummy-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | ||||||||||||
- name: Save spec/dummy/node_modules to cache | ||||||||||||
uses: actions/cache@v3 | ||||||||||||
with: | ||||||||||||
path: spec/dummy/node_modules | ||||||||||||
key: v5-dummy-app-node-modules-cache-${{ hashFiles('spec/dummy/yarn.lock') }} | ||||||||||||
key: dummy-app-node-modules-cache-${{ hashFiles('spec/dummy/package.json') }}-${{ matrix.versions }} | ||||||||||||
- id: get-sha | ||||||||||||
run: echo "::set-output name=sha::$(git rev-parse HEAD)" | ||||||||||||
- name: Save test webpack bundles to cache (for build number checksum used by rspec job) | ||||||||||||
uses: actions/cache@v3 | ||||||||||||
with: | ||||||||||||
path: spec/dummy/public/webpack | ||||||||||||
key: v5-dummy-app-webpack-bundle-${{ steps.get-sha.outputs.sha }} | ||||||||||||
key: dummy-app-webpack-bundle-${{ steps.get-sha.outputs.sha }}-${{ matrix.versions }} | ||||||||||||
- name: Install Node modules with Yarn for renderer package | ||||||||||||
run: | | ||||||||||||
yarn install --no-progress --no-emoji | ||||||||||||
yarn run eslint -v | ||||||||||||
sudo yarn global add yalc | ||||||||||||
- name: yalc publish for react-on-rails | ||||||||||||
run: yalc publish | ||||||||||||
- name: yalc add react-on-rails | ||||||||||||
run: cd spec/dummy && yalc add react-on-rails | ||||||||||||
- run: cd spec/dummy && yarn add "shakapacker@${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}" | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification Potential issue with Shakapacker version addition and dynamic Gemfile update. The
Analysis chainPotential issue with Shakapacker version addition and dynamic Gemfile update. The repeated addition of Shakapacker versions and dynamic Gemfile updates could lead to maintenance challenges. Ensure that these operations are idempotent and do not introduce state inconsistencies across CI runs. Also applies to: 149-149 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify idempotence of dynamic Gemfile updates.
# Test: Run the Gemfile update multiple times and check for duplicate entries.
for i in {1..3}; do
echo "gem 'shakapacker', '~> 8.0.0'" >> Gemfile
done
grep "gem 'shakapacker'" Gemfile | wc -l
Length of output: 257 Script: #!/bin/bash
# Description: Verify idempotence of `yarn add` in the workflow script.
# Check if `yarn add` is used in a way that could introduce duplicate dependencies.
grep -E "yarn add" .github/workflows/main.yml
Length of output: 263 |
||||||||||||
- name: Dynamically add the right shakapacker version to Gemfiles | ||||||||||||
run: | | ||||||||||||
echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> Gemfile | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have https://github.com/shakacode/react_on_rails/blob/master/Gemfile.development_dependencies#L3 use the latest, and use a couple lines of bash code to change to the 6.6.0 when needed. |
||||||||||||
echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> spec/dummy/Gemfile | ||||||||||||
- name: Install Ruby Gems for package | ||||||||||||
run: bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 | ||||||||||||
- name: Install Ruby Gems for dummy app | ||||||||||||
|
@@ -164,8 +170,14 @@ jobs: | |||||||||||
run: echo fs.inotify.max_user_watches=524288 | sudo tee -a /etc/sysctl.conf && sudo sysctl -p | ||||||||||||
- name: generate file system-based packs | ||||||||||||
run: cd spec/dummy && RAILS_ENV=test bundle exec rake react_on_rails:generate_packs | ||||||||||||
- name: Git Stuff | ||||||||||||
run: | | ||||||||||||
git config user.email "[email protected]" | ||||||||||||
git config user.name "Your Name" | ||||||||||||
git commit -am "stop generators from complaining about uncommitted code" | ||||||||||||
- run: cd spec/dummy && bundle info shakapacker | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider the implications of committing within CI. Committing changes within a CI workflow can lead to unexpected behavior and should generally be avoided unless specifically intended as part of the workflow. |
||||||||||||
- name: Main CI | ||||||||||||
run: bundle exec rake run_rspec:all_but_examples | ||||||||||||
run: bundle exec rake run_rspec:all_dummy | ||||||||||||
- name: Store test results | ||||||||||||
uses: actions/upload-artifact@v3 | ||||||||||||
with: | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,11 +7,10 @@ on: | |
pull_request: | ||
|
||
jobs: | ||
build: | ||
rspec-package-tests: | ||
strategy: | ||
matrix: | ||
ruby: [3.0, 3.3] | ||
node: [16, 20] | ||
versions: ['oldest', 'newest'] | ||
runs-on: ubuntu-22.04 | ||
steps: | ||
- uses: actions/checkout@v4 | ||
|
@@ -20,7 +19,7 @@ jobs: | |
- name: Setup Ruby | ||
uses: ruby/setup-ruby@v1 | ||
with: | ||
ruby-version: ${{ matrix.ruby }} | ||
ruby-version: ${{ matrix.versions == 'oldest' && '3.0' || '3.3' }} | ||
bundler: 2.5.9 | ||
- name: Print system information | ||
run: | | ||
|
@@ -35,9 +34,16 @@ jobs: | |
uses: actions/cache@v3 | ||
with: | ||
path: vendor/bundle | ||
key: v5-package-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }} | ||
key: package-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ matrix.versions }} | ||
- name: Dynamically add the right shakapacker version to the Gemfile | ||
run: echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> Gemfile | ||
- name: Install Ruby Gems for package | ||
run: bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 | ||
- name: Git Stuff | ||
run: | | ||
git config user.email "[email protected]" | ||
git config user.name "Your Name" | ||
git commit -am "stop generators from complaining about uncommitted code" | ||
- name: Run rspec tests | ||
run: bundle exec rspec spec/react_on_rails | ||
- name: Store test results | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -21,10 +21,14 @@ Changes since the last non-beta release. | |||||||||
#### Fixed | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure proper spacing around headings and lists for better readability and markdown linting. +
#### Fixed
- Resolves possible thrown error for precompile checks with Shakapacker. [PR 1622](https://github.com/shakacode/react_on_rails/pull/1622) by [adriangohjw](https://github.com/adriangohjw)
+
+
- Generator errors with Shakapacker v8+ fixed [PR 1629](https://github.com/shakacode/react_on_rails/pull/1629) by [vaukalak]
+ Also applies to: 27-27 ToolsMarkdownlint
|
||||||||||
- Address a number of typos and grammar mistakes [PR 1631](https://github.com/shakacode/react_on_rails/pull/1631) by [G-Rath](https://github.com/G-Rath) | ||||||||||
|
||||||||||
#### Added | ||||||||||
|
||||||||||
- Adds an adapter module & improves test suite to support all versions of Shakapacker. [PR 1622](https://github.com/shakacode/react_on_rails/pull/1622) by [adriangohjw](https://github.com/adriangohjw) | ||||||||||
|
||||||||||
### [14.0.2] - 2024-06-11 | ||||||||||
|
||||||||||
#### Fixed | ||||||||||
- Project initialization with Shakapacker v8+ fixed [PR 1629](https://github.com/shakacode/react_on_rails/pull/1629) by [vaukalak](https://github.com/vaukalak) | ||||||||||
- Generator errors with Shakapacker v8+ fixed [PR 1629](https://github.com/shakacode/react_on_rails/pull/1629) by [vaukalak](https://github.com/vaukalak) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure proper spacing around lists for better readability and markdown linting. +
- Generator errors with Shakapacker v8+ fixed [PR 1629](https://github.com/shakacode/react_on_rails/pull/1629) by [vaukalak]
+ Committable suggestion
Suggested change
ToolsMarkdownlint
|
||||||||||
|
||||||||||
### [14.0.1] - 2024-05-16 | ||||||||||
|
||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -132,17 +132,6 @@ yarn | |||||||||||
yarn build | ||||||||||||
``` | ||||||||||||
|
||||||||||||
Or run this which builds the yarn package, then the webpack files for spec/dummy, and runs tests in | ||||||||||||
spec/dummy. | ||||||||||||
|
||||||||||||
|
||||||||||||
```sh | ||||||||||||
# Optionally change default selenium_firefox driver | ||||||||||||
export DRIVER=selenium_firefox | ||||||||||||
cd react_on_rails/ | ||||||||||||
yarn run dummy:spec | ||||||||||||
``` | ||||||||||||
|
||||||||||||
### Run NPM JS tests | ||||||||||||
|
||||||||||||
```sh | ||||||||||||
|
@@ -153,11 +142,17 @@ yarn test | |||||||||||
### Run spec/dummy tests | ||||||||||||
|
||||||||||||
```sh | ||||||||||||
cd react_on_rails/spec/dummy | ||||||||||||
cd react_on_rails | ||||||||||||
npm -g i yalc | ||||||||||||
bundle && yarn | ||||||||||||
yalc publish | ||||||||||||
cd spec/dummy | ||||||||||||
bundle && yarn | ||||||||||||
yarn add shakapacker | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add instructions on how to locally test the older shakapacker |
||||||||||||
rspec | ||||||||||||
``` | ||||||||||||
|
||||||||||||
### Run most tests and linting | ||||||||||||
### Run package tests and linting | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a section for running package tests and linting enhances the documentation. Consider providing more details about what + ### More on Running Package Tests and Linting
+ This section will cover what `yarn run check` includes, such as which tests and linters are run, and any additional setup required. Committable suggestion
Suggested change
|
||||||||||||
|
||||||||||||
```sh | ||||||||||||
cd react_on_rails/ | ||||||||||||
|
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.
Tip
Codebase Verification
Inconsistent Caching Strategy Across Workflows
Upon reviewing the caching keys across different workflow files, we have identified multiple inconsistencies:
dummy-app-node-modules-cache-${{ hashFiles('spec/dummy/package.json') }}-${{ matrix.versions }}
andv5-package-node-modules-cache-${{ hashFiles('yarn.lock') }}
inmain.yml
.Please address the following:
Files to review:
.github/workflows/main.yml
.github/workflows/rspec-package-specs.yml
.github/workflows/examples.yml
.github/workflows/package-js-tests.yml
.github/workflows/lint-js-and-ruby.yml
Analysis chain
Check consistency in caching strategy.
Ensure the caching keys are consistent across different workflows and reflect the actual dependencies being installed. This change appears correct, but verifying consistency across workflows would be beneficial.
Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 2126