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

Don't emit error when registering for event 'signin ready' #784

Merged
merged 5 commits into from
Jan 11, 2017

Conversation

theopak
Copy link
Contributor

@theopak theopak commented Jan 9, 2017

Problem

Looks like Lock v10.8.0 added event validation via #756. However this feature emits error messages for many events that Lock makes available (e.g. those events from auth0/docs#191).

This bug occurs on all supported browsers and operating systems since at least the v10.8.0 release, including the latest version today.

Fix

I simply added 'signin ready' to the whitelist of valid events: https://github.com/auth0/lock/blob/master/src/core.js#L36

There are other events in auth0/docs#191 that have the same issue but for my purposes I only need 'signin ready' fixed in order to unblock my production issue.

When I build this branch locally I no longer see the 'Invalid event' console error.

Issue Reproduction

The following code in my application results in the error Invalid event "signin ready"..

Here's the same code in a codepen so you can confirm that 'Invalid event' shows up in the browser console: http://codepen.io/theopak/pen/QdjVZX

lock.show()

// This function adds a message to the Lock.js dialog box, 
// as a proof-of-concept of something that can only be done
// after the 'signin ready' event is triggered.
const addMessageToLock = (event) => {
  let myDiv = document.createElement('h4')
  myDiv.style.textAlign = 'center'
  myDiv.style.textAlign = 'center'
  myDiv.innerHTML = 'Example Message Added to Lock'
  const targetNode = document.getElementsByClassName('auth0-lock-input-block')[0].parentNode
  targetNode.insertBefore(myDiv, targetNode.firstChild)
}

// Here's an example of how the 'signin ready' event is used
// to do something that's only possible after Lock.js is in 
// the DOM and done appearing.
lock.on('signin ready', addMessageToLock)

@theopak
Copy link
Contributor Author

theopak commented Jan 9, 2017

Hey Auth0 team, it would be much appreciated if you could consider this minor fix to be included on both a v10.9.x and v10.8.x release ASAP because it's causing some annoyance among teams at our company that have become dependent on Lock's 'signin ready' event in order to apply a specific customization that we need. Thanks!

Copy link

@hsingh23 hsingh23 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@@ -39,7 +39,8 @@ export default class Base extends EventEmitter {
'unrecoverable_error',
'authenticated',
'authorization_error',
'hash_parsed'
'hash_parsed',
'signin ready'
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add signup ready too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, done

@glena
Copy link
Contributor

glena commented Jan 9, 2017

This will impact lock 10.9. I think the safer option for you is to use the previous release to the one that is 10.7.2.

@hzalaz
Copy link
Member

hzalaz commented Jan 9, 2017

Also this is sort of an undocumented event, while I understand the use case and need, we will review it to see if it works properly since we didn't intended it to be publicly used.

Once we make sure it's working properly and documented, we'll release it for v10.9.x

@theopak
Copy link
Contributor Author

theopak commented Jan 9, 2017

I think the safer option for you is to use the previous release to the one that is 10.7.2.

That works too, although then we'd have other issues that were already fixed. v10.8.0 fixing #543 was especially nice!

This will impact lock 10.9.

Do you mean that, if merged, this could be included in a v10.9.x release? Sounds good to me.

@theopak
Copy link
Contributor Author

theopak commented Jan 9, 2017

Also this is sort of an undocumented event, while I understand the use case and need, we will review it to see if it works properly since we didn't intended it to be publicly used.

@hzalaz Thanks! As background information, I first found this event from a documentation page under https://auth0.com/docs/libraries/lock. IIRC that doc was lost when Lock v10 came out although you can see the 'signin ready' event advertised in an old commit of the page: https://github.com/auth0/docs/blob/5746f715c00785e47a5eb5e9d47fc41e95b45451/articles/libraries/lock/events.md

@glena
Copy link
Contributor

glena commented Jan 9, 2017

Yes, that is the documentation of lock < 10

@theopak
Copy link
Contributor Author

theopak commented Jan 11, 2017

Hi @glena I believe I made the change you requested. Could you review this PR again when you have a minute?

@hzalaz hzalaz added this to the v10-Next milestone Jan 11, 2017
@hzalaz hzalaz changed the title Don't emit error for event 'signin ready' Don't emit error when registering for event 'signin ready' Jan 11, 2017
@glena glena modified the milestones: v10-Next, v10.9.2 Jan 11, 2017
@glena glena merged commit 802fc8f into auth0:master Jan 11, 2017
@glena
Copy link
Contributor

glena commented Jan 11, 2017

released in 10.9.2

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.

4 participants