build: dont check submodules to build module #1927
Conversation
The only git submodule we need is ably-common, and this is only used as part of testing to create test apps. We don't use it when building (despite it being a build check). This check during build prevents other SDKs from depending on a branch of ably-js, as it doesn't have the submodules, and can't init them. This change therefore removes the submodules check when building the library, allowing SDKs to depend on branch versions, if they want to.
WalkthroughThe changes involve a modification to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Grunt
participant Webpack
participant Build
User->>Grunt: Execute build task
Grunt->>Webpack: Run webpack:all
Grunt->>Build: Run build:browser
Grunt->>Build: Run build:node
Grunt->>Build: Run build:push
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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
🧹 Outside diff range and nitpick comments (1)
Gruntfile.js (1)
Line range hint
44-60: Consider improving submodule check error messages.Since the submodule is now only required for testing, the error messages could be more specific to avoid confusion:
grunt.log.writeln('%s : while checking submodule path!', error.message); - grunt.log.writeln('Probably, git submodule at %s are not initialized?', pathToSubmodule); - grunt.log.writeln('Please, initialize it with `git submodule init & git submodule update`!'); + grunt.log.writeln('Git submodule at %s is required for running tests but not for building.', pathToSubmodule); + grunt.log.writeln('If you need to run tests, initialize it with `git submodule init & git submodule update`'); return done(false); } if (stats.isDirectory()) { grunt.log.writeln('Git submodule at %s is found!', pathToSubmodule); return done(); } grunt.log.writeln('Git submodule at %s is not initialized!', pathToSubmodule); - grunt.log.writeln('Please, initialize it with `git submodule init & git submodule update`!'); + grunt.log.writeln('This is only required for running tests. If you need to run tests, initialize it with `git submodule init & git submodule update`');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Gruntfile.js(1 hunks)
🔇 Additional comments (2)
Gruntfile.js (2)
76-76: LGTM! Build task modification aligns with PR objectives.
The removal of the checkGitSubmodules task from the build sequence successfully eliminates the submodule dependency during build, which should resolve the issues faced by SDKs depending on specific branches.
Line range hint 134-139: Verify retention of submodule check in test:webserver task.
The checkGitSubmodules task remains in the test:webserver task, which aligns with the PR objectives since the submodule is still required for testing. This appears intentional, but please confirm.
✅ Verification successful
Retention of checkGitSubmodules in test:webserver task is correctly implemented
The verification confirms that checkGitSubmodules is intentionally and correctly retained only in the test:webserver task, while being removed from the build process. This aligns with the task dependencies where:
test:webserverrequires the submodule for testing purposes- The task is properly sequenced after browser builds and before mocha tests
- No other build tasks depend on the submodule check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all remaining uses of checkGitSubmodules to ensure they're intentionally retained
rg "checkGitSubmodules" -A 2 -B 2
Length of output: 512
Script:
#!/bin/bash
# Let's verify the PR description and changes to understand the context better
gh pr view --json title,body
# Also check the changes in the Gruntfile to see the complete picture
rg "test:webserver|build:" -A 2 -B 2 Gruntfile.js
Length of output: 761
|
Have you confirmed that, with this change, you're successfully able to depend on a branch of ably-js? |
Yes - ably/ably-chat-js#417 fixed a few broken tests because it brought in new changes not yet released :) |
The only git submodule we need is ably-common, and this is only used as part of testing to create test apps. We don't use it when building (despite it being a build check).
This check during build prevents other SDKs from depending on a branch of ably-js, as it doesn't have the submodules, and can't init them.
This change therefore removes the submodules check when building the library, allowing SDKs to depend on branch versions, if they want to.
Summary by CodeRabbit
This change enhances the overall efficiency of the build process for users.