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

Proposal: Reduce bundle size and add a bundler for Stimulus Reflex javascript #315

Merged
merged 5 commits into from
Oct 18, 2020

Conversation

KonnorRogers
Copy link
Contributor

@KonnorRogers KonnorRogers commented Sep 23, 2020

Type of PR (feature, enhancement, bug fix, etc.)

Proposal

Description

Move @rails/actioncable, stimulus, and cable_ready to peerDependencies

Add cable_ready to the install task.

Add microbundle to bundle javascript for publishing.

Why should this be added

By moving these dependencies to peerDependencies, it will reduce the size of StimulusReflex's package and remove any possible version issues between user defined packages and the packages SR ships.

Adding a Bundler helps SR run in different environments.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@KonnorRogers KonnorRogers changed the title Refactor/package.json Refactor: Reduce bundle size and add a bundler for Stimulus Reflex javascript Sep 23, 2020
@KonnorRogers KonnorRogers changed the title Refactor: Reduce bundle size and add a bundler for Stimulus Reflex javascript Proposal: Reduce bundle size and add a bundler for Stimulus Reflex javascript Sep 23, 2020
@leastbad
Copy link
Contributor

Even your git commit messages are top notch

Readd the packages as devDependencies so they get installed during
development. (PeerDependencies are not automatically installed)
@leastbad
Copy link
Contributor

leastbad commented Oct 9, 2020

FYI: I have great faith in @ParamagicDev but I haven't tested this PR and am not 100% confident I could explain the ramifications to someone in a bar. @hopsoft what is your confidence interval?

@KonnorRogers
Copy link
Contributor Author

@leastbad So here's my $0.02, to start, yes there are trade offs.

In summary by moving ActionCable, CableReady, and Stimulus to peerDependencies, it means the user has to install them and it uses whatever the user has installed. If it does not match the version or the peerDependency, it will output a warning to the user when they yarn install.

The trade off is StimulusReflex does not have explicit versioning, however, it also means that users don't have 2 instances of ActionCable, CableReady, and Stimulus if they installed them in their project separate of StimulusReflex.

@leastbad leastbad merged commit 62f639b into stimulusreflex:master Oct 18, 2020
@leastbad
Copy link
Contributor

Thanks to @ParamagicDev, I'm now on a peerDependencies roll.

marcoroth added a commit to marcoroth/cable_ready that referenced this pull request Nov 3, 2020
Introduce `@cable_ready/polyfills`

Move javascript/ to packages/@cable_ready/core

Introduce microbundle for the package builds

Applies all of the changes from stimulusreflex/stimulus_reflex#340, stimulusreflex/stimulus_reflex#315 and stimulusreflex/stimulus_reflex#345 to cable_ready
marcoroth added a commit to marcoroth/stimulus_reflex that referenced this pull request Nov 5, 2020
This commit enforces users to add `cable_ready` as an explicit dependendcy to their app's `Gemfile`

This is to match the behavior introduced in stimulusreflex#315. Moving `cable_ready` to the `peerDependencies` resulted in that users now need to add `cable_ready` as a dependecy to the app's `package.json`

Co-Authored-By: paramagicdev <[email protected]>
marcoroth added a commit to marcoroth/stimulus_reflex that referenced this pull request Nov 5, 2020
This commit enforces users to add `cable_ready` as an explicit dependendcy to their app's `Gemfile`

This is to match the behavior introduced in stimulusreflex#315. Moving `cable_ready` to the `peerDependencies` resulted in that users now need to add `cable_ready` as a dependecy to the app's `package.json`

Co-Authored-By: paramagicdev <[email protected]>
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