Skip to content

authCV initialized to null, getter used in checkAuth#340

Merged
DarkPurple141 merged 8 commits intodevfrom
authcv-prerender-fix
Feb 7, 2019
Merged

authCV initialized to null, getter used in checkAuth#340
DarkPurple141 merged 8 commits intodevfrom
authcv-prerender-fix

Conversation

@NunoDasNeves
Copy link
Collaborator

idk how to enable prerender on staging - it seems like it's enabled everywhere according to src/backend/index.js ??

@coveralls
Copy link

coveralls commented Feb 7, 2019

Pull Request Test Coverage Report for Build 1582

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 86.314%

Files with Coverage Reduction New Missed Lines %
src/index.js 3 87.5%
Totals Coverage Status
Change from base Build 1560: -0.01%
Covered Lines: 681
Relevant Lines: 741

💛 - Coveralls

loading: ({ loading }) => loading,
error: ({ error }) => error,
authCV: ({ authCV }) => authCV
authCV: ({ authCV }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't work. It'll just alter the param, not the underlying CV.

I suggest this:

const _authCV = new CV()

const state = {...}

const getters = { authCV: () => _authCV }
const mutations = { ...SIGNAL_AUTH_CV() { _authCV.signal() }}

Copy link
Collaborator Author

@NunoDasNeves NunoDasNeves Feb 7, 2019

Choose a reason for hiding this comment

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

can't i just not destructure state? that seems to work

  authCV: (state) => {
    if (state.authCV === null) state.authCV = new CV()
    return state.authCV
  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also this is why we should be using Rust for our frontend ^_^

profile: null,
// condition variable for app to wait on while waiting for auth to resolve on boot
authCV: new CV()
authCV: null
Copy link
Collaborator

Choose a reason for hiding this comment

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

and then just remove this.


// signal the CV so the app can continue loading and use the JWT token in its requests
// Note we _need_ to do this before returning!
commit('SIGNAL_AUTH_CV')
Copy link
Collaborator

Choose a reason for hiding this comment

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

And then retain this as a commit

@DarkPurple141 DarkPurple141 added the Bug Something isn't working label Feb 7, 2019
@NunoDasNeves
Copy link
Collaborator Author

I mean I guess. Seems like violation of getters being non mutating. Also not sure if authCV is really application state.

hok then

@DarkPurple141
Copy link
Collaborator

@NunoDasNeves also the previous deploy was broken so 👍

@DarkPurple141 DarkPurple141 self-requested a review February 7, 2019 03:59
DarkPurple141
DarkPurple141 previously approved these changes Feb 7, 2019
Copy link
Collaborator

@DarkPurple141 DarkPurple141 left a comment

Choose a reason for hiding this comment

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

Could work.

Seems that getters which are calling mappers directly when object can be null are causing potential issues.

postProcess (renderedRoute) {
// Ignore any redirects.
renderedRoute.route = renderedRoute.originalRoute
// Basic whitespace removal. (Don't use this in production.)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't use in production? o.O

// Ignore any redirects.
renderedRoute.route = renderedRoute.originalRoute
// Basic whitespace removal. (Don't use this in production.)
renderedRoute.html = renderedRoute.html.split(/<div id="app"/)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this has been added to index.html already

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh this is a quote I copy-pasta'd. Just wanted to see if this works to stop re-render.

VUE_APP_TITLE=SmartCourse
VUE_APP_API_URL=https://smartcourse.me/api No newline at end of file
VUE_APP_API_URL=https://smartcourse.me/api
VUE_APP_URL=https://smartcourse-staging.azurewebsites.net No newline at end of file
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uhh is this right?

NODE_ENV=staging
VUE_APP_TITLE=SmartCourse (staging)
VUE_APP_API_URL=https://smartcourse-staging.azurewebsites.net/api
VUE_APP_URL=https://smartcourse.me
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think you got these switcharoo'd

@DarkPurple141 DarkPurple141 merged commit 26e5860 into dev Feb 7, 2019
@DarkPurple141 DarkPurple141 deleted the authcv-prerender-fix branch February 7, 2019 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants