-
Notifications
You must be signed in to change notification settings - Fork 493
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
Upgrading to 9.7.2 causes test breakages #806
Comments
What's the error? I think your string was trimmed. |
My bad:
|
From what I can see, it appears that your code is:
Either one of the two will fix this |
OK. I added this to my jest config:
Which fixed it. Out curiosity, how would I use the bundled version with React? |
9.7.0 should've been a major version bump, as any application participating in SSR is now broken. I just got bit by this in a Next.js application. |
@Timer It wasn’t our intention to break apps and I want to fix this in a patch release if we did that. I’ll recreate this scenario tomorrow to make sure it still works. My understanding of this is that apps would get the “browser“ npm entry and would all work out. But, as you pointed out, maybe not all apps are doing that. Just so you can help me testing that: does this issue happen on a brand new next js app + auth0js? |
Great to hear, @lucascrespo! Looking forward to a fix. 😄 Apps will look at the General rule of thumb is as follows:
Most people opt for Here's a really simple repro case: $ npx create-next-app example
$ cd example/
$ yarn add auth0-js
$ echo "import Auth0 from 'auth0-js'
export default () => <div />
" > pages/index.js
$ yarn dev
$
$ # new terminal
$
$ curl -s http://localhost:3000 | head -n5 | tail -n4
(function (exports, require, module, __filename, __dirname) { import Authentication from './authentication'; ^^^^^^
SyntaxError: Unexpected token import
$ yarn add auth0-js@9.6.x
$ curl -s http://localhost:3000 | head -n1
<!DOCTYPE html><html><head><meta charSet="utf-8" class="next-head"/><link rel="preload" href="/_next/-/page/index.js" as="script"/><link rel="preload" href="/_next/-/page/_app.js" as="script"/><link rel="preloa
d" href="/_next/-/page/_error.js" as="script"/><link rel="preload" href="/_next/static/commons/manifest.js" as="script"/><link rel="preload" href="/_next/static/commons/main.js" as="script"/></head><body><div i
d="__next"><div></div></div><div id="__next-error"></div><script> |
Thanks! That's very helpful. I'll dig through this today and I hope to have a patch by tomorrow. |
In order to be able to properly do tree-shaking, we need to start using ES modules (`import` / `export`) instead of CommonJS modules (`require`). This PR is a find/replace in all files, making them use import/export. This, of course, broke all the tests 😬. `mocha`, our test runner, doesn't support modern javascript, so I needed to add `babel` to transpile the code to compatible javascript during testing. ### Summary of changes: - Added `babel` to transpile test code in order to make `mocha` work - Upgraded eslint auth0 config so it allows modern javascript. Since we're not migrating our code yet, I had to turn off [a bunch of rules](https://github.com/auth0/auth0.js/pull/774/files#diff-1dc6ee56b778cd91e0327b52aaeaa1b9R19) - change istanbul (our test coverage runner) to nyc. nyc is pretty much istanbul 2.0 (see [here](istanbuljs/nyc#524 (comment))) - upgraded prettier, so we might have a few automatic styling changes - upgrade circle-ci to node@v8.9.0 because of a dependency
- Remove rollup-plugin-uglify because it doesn't support minifying ES6+. Using rollup-plugin-terser instead - Don't generate prod files when in dev mode - instead of using main/browser in package.json, using module/main (ES modules, UMD bundle) - This follows best practices outlined [here](#806 (comment)), [here](http://2ality.com/2017/04/setting-up-multi-platform-packages.html#solutions) and [here](https://github.com/rollup/rollup-starter-lib/blob/master/package.json#L5-L6).
Hey folks! With your help, I think I fixed this. Can you help me confirm that the beta version works for you?
|
Confirmed, my previously broken tests are now passing on @luisrudge Thank you for fixing! |
Ok thanks @murtyjones. Let's wait for @Timer and then I'll release 9.7.3 |
Hey @luisrudge! Just saw this notification, I can check within the next 8 hours if you want to wait for my LGTM. Looking at Unpkg though, this looks good! |
ok thanks @Timer |
Yup, my app is working again! Thanks @luisrudge. |
On a side note, I'm now seeing this in my console during SSR:
|
@Timer I think you'd get the same warning in the previous version, right? |
Yeah, it might've been a Next.js update that surfaced that warning now. Just an FYI -- everything still works when it gets to the client side. 😄 |
@Timer I just tested with the 9.6.1 version and the warning doesn't happen. This happens because, previously, you'd import the raw code of auth0.js + dependencies and now you import a bundled file, which is already bundled with the browser version of superagent. That's why you see the browser warning, which makes sense. I'll release this on Monday! Thanks for the help! |
@luisrudge I was originally getting the exact same error as everyone mentioned earlier -- thanks for fixing that! However, now I'm getting a new, similar error when importing Auth0Lock (and I do have the new 9.7.3 Auth0.js installed). From the stack trace, it seems related to the line/file you were touching with auth0/lock#1442 - perhaps Lock needs a new release as well?
|
Hi @coreymason! We just released Lock@11.8.0 which has an updated version of auth0-js. Can you try that again? Thanks! |
I upgraded from auth0-js 9.5.1 to ^9.7.2 and am now seeing the following error when running my jest tests:
The text was updated successfully, but these errors were encountered: