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

Major updates and refactors #12

Merged
merged 4 commits into from
Feb 16, 2020
Merged

Major updates and refactors #12

merged 4 commits into from
Feb 16, 2020

Conversation

jmwohl
Copy link
Member

@jmwohl jmwohl commented Feb 8, 2020

  • updates dependencies
  • updated to more current JS standards
  • updates webpack build process
  • lots of fixes and refactoring in order to work with updated dependencies

Known issues:

  • The notifications package I was using hasn't been maintained and doesn't work, so either needs to be forked and fixed or swapped out for another package

@jmwohl jmwohl mentioned this pull request Feb 8, 2020
apiBase: 'http://localhost:8888/v0/',
apiBase: 'https://api.openframe.io/v0/',
Copy link
Member Author

Choose a reason for hiding this comment

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

@jvolker Notice this — this points the local dev web app at the production api. If/when we get the API server running locally again we should switch this default back.

Might be better long-term to move these settings to env vars.

@@ -112,41 +127,42 @@ class App extends Component {
{
//<Notification notice={ ui.notice } updateNotification={actions.updateNotification}/>
}
<Notifs />
{/* <Notifs /> */}
Copy link
Member Author

Choose a reason for hiding this comment

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

This Notifs (from redux-notifications) wasn't easy to just update along with the other deps... we'll need an alternative, to fork and update this package, or to roll our own little notifications lib.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the exact functionality we would be loosing with this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that all notifications outside overlays?
What's the problem with redux-notifications?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for the success/info/error notifications for the UI (not the error warnings in edit forms).

I don't remember exactly what the error was, but think it hasn't been updated to work with the latest versions of react and/or redux. Maybe be that we can fork and update it or find another comparable package.

@jmwohl jmwohl requested a review from jvolker February 9, 2020 14:59
@jmwohl
Copy link
Member Author

jmwohl commented Feb 9, 2020

@jvolker As I mentioned in slack, don't worry too much about reviewing this in detail. It's WAY too big of a PR to review in a comprehensive way, and I don't plan on releasing this version until it's well tested, so I imagine there will be a number of bug fix and update PRs after this before we'd release this code.

@jvolker
Copy link
Collaborator

jvolker commented Feb 9, 2020

Thanks so much for this big update @jmwohl. I'm going to do review it later. I can already confirm that I can run this locally without errors using npm start.

const open = require('open');

let server = new WebpackDevServer(webpack(config), config.devServer);

server.listen(config.port, '0.0.0.0', (err) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a particular reason why you hardcoded the port?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope! I don't remember why I did that, and there's no reason it can't come from the config. I'll update that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Realized this file isn't actually used, removed it.

Remove duplicate comment.
@jvolker
Copy link
Collaborator

jvolker commented Feb 13, 2020

Editing artwork and saving it without a name crashes the website and throws this error:

backend.js:6 The above error occurred in the <div> component:
    in div (created by ArtworkEditArtworkModalComponent)
    in div (created by ArtworkEditArtworkModalComponent)
    in div (created by ArtworkEditArtworkModalComponent)
    in div (created by ArtworkEditArtworkModalComponent)
    in div (created by ModalPortal)
    in div (created by ModalPortal)
    in ModalPortal (created by Modal)
    in Modal (created by ArtworkEditArtworkModalComponent)
    in div (created by ArtworkEditArtworkModalComponent)
    in ArtworkEditArtworkModalComponent (created by ModalManagerContainer)
    in div (created by ModalManagerContainer)
    in ModalManagerContainer (created by ConnectFunction)
    in ConnectFunction (created by App)
    in div (created by App)
    in App (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(App))
    in Router
    in Provider

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.
r @ backend.js:6
logCapturedError @ react-dom.development.js:21843
logError @ react-dom.development.js:21880
update.callback @ react-dom.development.js:23232
callCallback @ react-dom.development.js:13829
commitUpdateEffects @ react-dom.development.js:13867
commitUpdateQueue @ react-dom.development.js:13857
commitLifeCycles @ react-dom.development.js:22160
commitLayoutEffects @ react-dom.development.js:25344
callCallback @ react-dom.development.js:336
invokeGuardedCallbackDev @ react-dom.development.js:385
invokeGuardedCallback @ react-dom.development.js:440
commitRootImpl @ react-dom.development.js:25082
unstable_runWithPriority @ scheduler.development.js:697
runWithPriority$2 @ react-dom.development.js:12149
commitRoot @ react-dom.development.js:24922
finishSyncRender @ react-dom.development.js:24329
performSyncWorkOnRoot @ react-dom.development.js:24307
eval @ react-dom.development.js:12199
unstable_runWithPriority @ scheduler.development.js:697
runWithPriority$2 @ react-dom.development.js:12149
flushSyncCallbackQueueImpl @ react-dom.development.js:12194
flushSyncCallbackQueue @ react-dom.development.js:12182
batchedUpdates$1 @ react-dom.development.js:24392
notify @ Subscription.js:26
notifyNestedSubs @ Subscription.js:68
handleChangeWrapper @ Subscription.js:73
dispatch @ redux.js:229
eval @ index.js:104
eval @ index.js:12
dispatch @ redux.js:644
eval @ updateArtworkRequest.js:19
Promise.then (async)
eval @ updateArtworkRequest.js:16
eval @ index.js:9
eval @ redux.js:483
_handleSubmitSaveArtwork @ ModalManagerContainer.js:159
executeSubmit @ handleSubmit.js:56
handleSubmit @ handleSubmit.js:149
Form._this.submit @ createReduxForm.js:366
callCallback @ react-dom.development.js:336
invokeGuardedCallbackDev @ react-dom.development.js:385
invokeGuardedCallback @ react-dom.development.js:440
invokeGuardedCallbackAndCatchFirstError @ react-dom.development.js:454
executeDispatch @ react-dom.development.js:584
executeDispatchesInOrder @ react-dom.development.js:609
executeDispatchesAndRelease @ react-dom.development.js:713
executeDispatchesAndReleaseTopLevel @ react-dom.development.js:722
forEachAccumulated @ react-dom.development.js:694
runEventsInBatch @ react-dom.development.js:739
runExtractedPluginEventsInBatch @ react-dom.development.js:880
handleTopLevel @ react-dom.development.js:5803
batchedEventUpdates$1 @ react-dom.development.js:24401
batchedEventUpdates @ react-dom.development.js:1415
dispatchEventForPluginEventSystem @ react-dom.development.js:5894
attemptToDispatchEvent @ react-dom.development.js:6010
dispatchEvent @ react-dom.development.js:5914
unstable_runWithPriority @ scheduler.development.js:697
runWithPriority$2 @ react-dom.development.js:12149
discreteUpdates$1 @ react-dom.development.js:24417
discreteUpdates @ react-dom.development.js:1438
dispatchDiscreteEvent @ react-dom.development.js:5881
Show 2 more frames
react-dom.development.js:14748 Uncaught (in promise) Error: Objects are not valid as a React child (found: Error: The `Artwork` instance is not valid. Details: `title` can't be blank (value: "").). If you meant to render a collection of children, use an array instead.
    in div (created by ArtworkEditArtworkModalComponent)
    in div (created by ArtworkEditArtworkModalComponent)
    in div (created by ArtworkEditArtworkModalComponent)
    in div (created by ArtworkEditArtworkModalComponent)
    in div (created by ModalPortal)
    in div (created by ModalPortal)
    in ModalPortal (created by Modal)
    in Modal (created by ArtworkEditArtworkModalComponent)
    in div (created by ArtworkEditArtworkModalComponent)
    in ArtworkEditArtworkModalComponent (created by ModalManagerContainer)
    in div (created by ModalManagerContainer)
    in ModalManagerContainer (created by ConnectFunction)
    in ConnectFunction (created by App)
    in div (created by App)
    in App (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(App))
    in Router
    in Provider
    at throwOnInvalidObjectType (react-dom.development.js:14748)
    at reconcileChildFibers (react-dom.development.js:15606)
    at reconcileChildren (react-dom.development.js:18096)
    at updateHostComponent (react-dom.development.js:18611)
    at beginWork$1 (react-dom.development.js:20193)
    at HTMLUnknownElement.callCallback (react-dom.development.js:336)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:385)
    at invokeGuardedCallback (react-dom.development.js:440)
    at beginWork$$1 (react-dom.development.js:25780)
    at performUnitOfWork (react-dom.development.js:24695)

Copy link
Collaborator

@jvolker jvolker left a comment

Choose a reason for hiding this comment

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

I skimmed the code as requested and ran the app to test manually for errors:

  • browse public and private channels (multiple artwork formats incl. shaders)
  • adding artwork
  • editing artwork
  • deleting artwork
  • edited frame
  • deleting frame

I've found a bug when editing artwork and added a few more comments.

Again, thanks a lot for this massive update!

@jvolker
Copy link
Collaborator

jvolker commented Feb 13, 2020

Closes #5

@jmwohl
Copy link
Member Author

jmwohl commented Feb 16, 2020

I fixed the artwork edit error, but there's definitely more work to do on normalizing our error handling in the UI.

@jmwohl
Copy link
Member Author

jmwohl commented Feb 16, 2020

Alright, I decided to just pull the redux-notifications code into our project rather than use the unmaintained dependency. I've updated the code a bit so we should have working notifications again now.

@jmwohl jmwohl merged commit c56c941 into master Feb 16, 2020
@jmwohl jmwohl deleted the npmupdates branch February 16, 2020 20:08
@jvolker
Copy link
Collaborator

jvolker commented Feb 16, 2020

Great, thanks for your work! I opened #15 to continue the conversation about notifications/alerts there.

@jvolker
Copy link
Collaborator

jvolker commented Feb 16, 2020

I fixed the artwork edit error, but there's definitely more work to do on normalizing our error handling in the UI.

I've just tested this and it's still an issue for me. I've tested with artwork "5e449d86a38167076035c032".

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.

2 participants