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

Inline util.format and replace usage of global #2030

Merged
merged 7 commits into from
Sep 7, 2021
Merged

Conversation

stevehobbsdev
Copy link
Contributor

Changes

This PR inlines the usage of util.format and removes the need to pull in the native util module. This fixes environments like Webpack 5 that no longer automatically polyfills such native modules.

It also replaces the usage of global with window, the again the defaults around providing this in Webpack 5 have changed. However, this library was incorrectly using global where it should have just used window.

References

Fixes #1999

Testing

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language

Checklist

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 3, 2021

This pull request introduces 2 alerts when merging a35337d into 2dd24fa - view on LGTM.com

new alerts:

  • 1 for Comparison between inconvertible types
  • 1 for Unneeded defensive code

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 3, 2021

This pull request introduces 2 alerts when merging 171ff6c into 2dd24fa - view on LGTM.com

new alerts:

  • 1 for Comparison between inconvertible types
  • 1 for Unneeded defensive code

@@ -5,7 +5,7 @@ exports[`HRDScreen Component renders correctly when there is an enterprise domai
data-__type="hrd_pane"
data-header={
<p>
Please enter your corporate credentials at [ 'domain.com' ].
Please enter your corporate credentials at domain.com.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming it's ok that these snapshots have changed? The new output looks better...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These appear to change depending on the version of node that generates the snapshots. I think with Node 10 you get the former version, but newer versions omit the wrapping [' ']. The output on the actual screen is the same though. These just snuck in from a previous check-in.

@stevehobbsdev stevehobbsdev merged commit 6c5a793 into master Sep 7, 2021
@stevehobbsdev stevehobbsdev deleted the fix/util branch September 7, 2021 15:23
@stevehobbsdev stevehobbsdev mentioned this pull request Sep 13, 2021
@GlebVST
Copy link

GlebVST commented Sep 15, 2021

Hey @stevehobbsdev so apparently this change from global to window could break some apps using auth0 lock (as it did our) in server-side-rendering environment (where you have no window basically), you guys sure this was good for a minor version change??

@stevehobbsdev
Copy link
Contributor Author

stevehobbsdev commented Sep 15, 2021

@GlebVST Sorry to hear that we may have inadvertently broken you; we believed it to be a fairly innocuous change given the use cases we support. Which SSR environment/framework are you using?

SSR isn't an officially supported use case for Lock; normally I would suggest configuring your app in such a way that it ignores Lock for the SSR build, if that's possible. In the meantime I would pin to 11.30.4 until a resoultion can be found.

@GlebVST
Copy link

GlebVST commented Sep 15, 2021

@stevehobbsdev err it's a bit of surprise to know SSR is not supported, after like 4 years we're using it ;) Yeah it's a bit dated React app where Lock is just used on a login component that gets rendered by node/express when people land on /login path - not quite sure why such usage would be out of support?
Right, rolled back to 11.30.4 for now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:medium Medium review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
3 participants