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

get rid of global notify #5355

Merged
merged 8 commits into from
Jul 12, 2018
Merged

Conversation

williaster
Copy link
Contributor

@williaster williaster commented Jul 6, 2018

This PR is necessary to move forward with lazy loading / webpack 4 / bundle optimization upgrade because globals break split bundles.

I removed all instances of the notify.xxx() calls and instead used the message toast components + redux tree. this was way more painful that it needed, we should never use globals for anything even if they are "easier."

@mistercrunch @michellethomas @graceguo-supercat

I tested sqllab, explore, dashboard v1, and dashboard v2 apps.

TODO

  • convert python flash messages to toasts

@williaster
Copy link
Contributor Author

williaster commented Jul 7, 2018

@mistercrunch I'm fixing tests, but I don't understand how SQL lab alerts worked previously, they aren't used by any sqllab component, they are only defined in App as a proptype, referenced in redux logic, and they don't seem to feed into the previous AlertsWrapper.

Thoughts?

@williaster
Copy link
Contributor Author

@mistercrunch @graceguo-supercat this should be good to go.

I made sure flash_messages are displayed, but I'm not sure that it works correctly because of the way we cache sqllab redux state. when I force the backend to add flash messages (they appear in the bootstrap data), the empty messages array from the previous state takes precedence and none are displayed. in any case, this was probably broken before (and still unsure about alerts, from my question above. any thoughts @mistercrunch ?)

@williaster
Copy link
Contributor Author

ping @graceguo-supercat @mistercrunch

@williaster
Copy link
Contributor Author

ping @graceguo-supercat @mistercrunch, this is blocking webpack 4 integration in #5370 as well.

@williaster
Copy link
Contributor Author

ping @graceguo-supercat @mistercrunch

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

@williaster williaster merged commit 19ac6e1 into apache:master Jul 12, 2018
@williaster williaster deleted the chris--get-rid-of-notify branch July 12, 2018 18:50
xumiao pushed a commit to xumiao/supernorm that referenced this pull request Jul 16, 2018
* [toasts] get rid of notify globals, refactor messageToasts for use by entire app

* [remove notify] use arrow func in ajax call

* fix lint + tests

* actually fix tests from messageToast refactor

* add 'test:one' npm script

* debugger

* [toasts] convert bootstrap flash messages to toasts in explore + sqllab

* [toasts][tests] import from right file
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Jul 19, 2018
* [toasts] get rid of notify globals, refactor messageToasts for use by entire app

* [remove notify] use arrow func in ajax call

* fix lint + tests

* actually fix tests from messageToast refactor

* add 'test:one' npm script

* debugger

* [toasts] convert bootstrap flash messages to toasts in explore + sqllab

* [toasts][tests] import from right file

(cherry picked from commit 19ac6e1)
timifasubaa pushed a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
* [toasts] get rid of notify globals, refactor messageToasts for use by entire app

* [remove notify] use arrow func in ajax call

* fix lint + tests

* actually fix tests from messageToast refactor

* add 'test:one' npm script

* debugger

* [toasts] convert bootstrap flash messages to toasts in explore + sqllab

* [toasts][tests] import from right file
williaster added a commit to airbnb/superset-fork that referenced this pull request Aug 6, 2018
* [toasts] get rid of notify globals, refactor messageToasts for use by entire app

* [remove notify] use arrow func in ajax call

* fix lint + tests

* actually fix tests from messageToast refactor

* add 'test:one' npm script

* debugger

* [toasts] convert bootstrap flash messages to toasts in explore + sqllab

* [toasts][tests] import from right file
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* [toasts] get rid of notify globals, refactor messageToasts for use by entire app

* [remove notify] use arrow func in ajax call

* fix lint + tests

* actually fix tests from messageToast refactor

* add 'test:one' npm script

* debugger

* [toasts] convert bootstrap flash messages to toasts in explore + sqllab

* [toasts][tests] import from right file
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants