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

make element.reflexController a dictionary #379

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

existentialmutt
Copy link
Contributor

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

bug fix

Description

Following up on #377, this PR makes element.reflexController a dictionary to prevent multiple reflexes on the same element from overwriting eachothers' data

Fixes # (issue)

Why should this be added

prevents race conditions when running multiple reflexes on the same element

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

Please note that the best way to suggest changes or updates to the documentation is to join Discord and leave a note in the #docs channel. Any documentation updates posted as PRs cannot be accepted at this time. ❤️

@leastbad
Copy link
Contributor

Hey Rafe, another good catch.

My understanding of things is that an element can only have one instance of a controller on it. That means we could solve this "simply" by removing line 56 of lifecycle.js, right?

I'm trying to wrap my head around a scenario where there would be multiple different Reflex controllers on an element with different names. We might have to ask @hopsoft for clarity here.

@existentialmutt
Copy link
Contributor Author

Good call- I suspect removing delete element.reflexController would do the trick. I'll test that next chance I get.

@hopsoft
Copy link
Contributor

hopsoft commented Nov 23, 2020

I'll have to think on this a bit harder, but the reason we call delete element.reflexController is to clean up references that might cause memory leaks, so we'd still want to remove any references to controllers embedded in the new dictionary datatype.

@leastbad
Copy link
Contributor

I usually shy away from shotgun solutions, but I have an idea:

if (reflexes.length == 1) delete element.reflexController

The premise here is that if there's only one active Reflex in the dictionary, it's the current one, so you're safe to clear out the controller reference.

@hopsoft
Copy link
Contributor

hopsoft commented Nov 24, 2020

Makes sense, but I expect that this should be sufficient. delete element.reflexController[reflexId]

@leastbad leastbad merged commit 81b420f into stimulusreflex:master Nov 24, 2020
julianrubisch added a commit to julianrubisch/stimulus_reflex that referenced this pull request Dec 25, 2020
* Exit with nonzero status code

* [nodoc] update changelog

* GitBook: [master] 3 pages modified

* [nodoc] update changelog

* Handles to mitigate race conditions when running reflexes in quick succession on the same element (stimulusreflex#377)

* element.reflexData becomes a dictionary

Co-authored-by: leastbad <[email protected]>

* [nodoc] update changelog

* fixed bug preventing callbacks for multiple morphs

* [nodoc] update changelog

* GitBook: [master] 5 pages modified

* GitBook: [master] one page modified

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* make element.reflexController a dictionary (stimulusreflex#379)

make element.reflexController a dictionary

* [nodoc] update changelog

* Move package.json to root of project (stimulusreflex#380)

* Move package.json to root of project
* Update readme instructions for publish

* [nodoc] update changelog

* Prep for v3.4.0.pre5 release

* Update to use gitcdn for logo on readme

* Bump js package version

* Return from received if isolationMode: true

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* non-SR cable_ready operation pass-through (stimulusreflex#381)

* works like a charm

* a superior solution

* [nodoc] update changelog

* Set reflexController to empty object

* 800

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* don't show findElement warnings unless debugging (stimulusreflex#384)

* dont exit in sanity checker on stimulus_reflex:install

* split SR operations from data.operations (stimulusreflex#385)

* [nodoc] update changelog

* [nodoc] update changelog

* add jQuery support to SR events (stimulusreflex#388)

* Update templates for new stage etc (stimulusreflex#390)

* updated templates for new lifecycle stages

* added accessors to Reflex class

* [nodoc] update changelog

* reflexError and received refactor (stimulusreflex#389)

* reflexError and received refactor

* strict equality checks + simpler existential checks

* Setup a proxy object that wraps CableReady::Channels (stimulusreflex#382)

Setup a proxy object that wraps CableReady::Channels

This improves developer ergonimics for emitting CR broadcasts on the SR
channel within a reflex.

* Remove codeclimate

* [nodoc] update changelog

* Add codacy config file

* [nodoc] update changelog

* Have codacy ignore markdown files

* Allow `success` and `after` lifecycle methods on replaced elements (stimulusreflex#386)

* Allow `success` and `after` lifecycle methods on replaced elements

* use destruction syntax

* Extract current debug setting into debug.js to sync the value between files

* Log warnings if users morphs the element which triggered the reflex.

* Log extra warning if the element is not present anymore to state, that the lifecycle methods were not executed

* standardize

* Tweaked wording slightly

* added reflexError handling to dispatchLifecycleEvent

Co-authored-by: leastbad <[email protected]>
Co-authored-by: leastbad <[email protected]>

* [nodoc] update changelog

* Prep for 3.4.0.pre6 release

* [nodoc] update changelog

* Update js package version

* [nodoc] update changelog

* Update %file_name%_reflex.rb.tt

* Update stimulus_reflex.js

* [nodoc] update changelog

* Update lifecycle.js

* tweaked life-cycle messages

* Add check to return early if element is undefined in invokeLifecycleMethod

* don't warn folks twice (stimulusreflex#392)

* [nodoc] update changelog

* [nodoc] update changelog

* Update stimulus_reflex.rb

* [nodoc] update changelog

* GitBook: [master] 12 pages and 11 assets modified

* Trigger piggy back operations after SR operations (stimulusreflex#395)

* Trigger piggy back operations after SR operations
* Refactor proxy object to be explicit about what is delegated.
* Update to latest CableReady

* [nodoc] update changelog

* Prep for 3.4.0.pre7

* [nodoc] update js package version to 3.4.0-pre7

* GitBook: [master] 3 pages modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* Delegate render to controller (stimulusreflex#397)

* Delegate render to controller

* Stand up basic testing for reflex rendering

* Delegate render to the controller class rather than instance

* Also, refactor some internals to match Rails internals.

* [nodoc] update changelog

* [nodoc] prepare for pre8

* [nodoc] update js package to pre8

* GitBook: [master] 14 pages modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] 14 pages modified

* [nodoc] update changelog

* GitBook: [master] 14 pages modified

* [nodoc] update changelog

* loosen stimulus dev dependency

* GitBook: [master] 4 pages modified

* [nodoc] update changelog

* GitBook: [master] 11 pages modified

* [nodoc] update changelog

* GitBook: [master] 2 pages modified

* [nodoc] update changelog

* GitBook: [master] 14 pages modified

* GitBook: [master] 2 pages modified

* GitBook: [master] one page modified

* an install that actually works on a fresh Rails new

* GitBook: [master] one page modified

* [nodoc] update changelog

* Don't memoize the singleton (stimulusreflex#400)

* [nodoc] update changelog

* GitBook: [master] 2 pages modified

* [nodoc] update changelog

* Add dom_id to the reflex (stimulusreflex#405)

* [nodoc] update changelog

* Check package version from yarn.lock if node_modules folder is not available. Fixex stimulusreflex#402 (stimulusreflex#403)

* [nodoc] update changelog

* Don't run sanity checker in production (stimulusreflex#404)

* Allow StimulusReflex to process Rack middlewares (stimulusreflex#399)

* Allow StimulusReflex to process Rack middleswares

* Possibly fixes stimulusreflex#289

* process the whole Rails middleware stack

* just call the Rails middleware stack if it reponds to `build`

* pass routes to middleware build method

* make middleware processing configurable

* use ActionDispatch::MiddlewareStack to register middlewares

* update comment in initializer

* add note so people know what to do if they encounter the 'No route matches' error

* Update channel.rb

* Update stimulus_reflex.rb

Co-authored-by: leastbad <[email protected]>

* [nodoc] prep for v3.4.0.pre9

* [nodoc] update js package version

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] 2 pages modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* Update README.md

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] 2 pages modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* [nodoc] prep for v3.4.0

* [nodoc] update npm package version

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* [nodoc] update changelog

* GitBook: [master] one page modified

* GitBook: [master] one page modified

* [nodoc] update changelog

* fix 'operartion' typo

* [nodoc] update changelog

* Update README.md

Co-authored-by: leastbad <[email protected]>
Co-authored-by: GitHub Actions <[email protected]>
Co-authored-by: leastbad <[email protected]>
Co-authored-by: Rafe Rosen <[email protected]>
Co-authored-by: Nate Hopkins <[email protected]>
Co-authored-by: Marco Roth <[email protected]>
Co-authored-by: Roland Studer <[email protected]>
Co-authored-by: Josh LeBlanc <[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