IdentityModel / oidc-token-manager

Apache License 2.0
51 stars 36 forks source link

Errors when using webpack #57

Closed maxmantz closed 8 years ago

maxmantz commented 8 years ago

I'm trying to use the token manager in an SPA built with ES6 & Webpack. I successfully ran npm install --save oidc-token-manager.

I can also include the file with webpack by using the standard import OIdcTokenManager from 'oidc-token-manager' syntax. However, when the file is loaded the following error message appears: OIdcClient is not defined. The docs say that the OidcClient is included in the package. Why does it not get loaded?

maxmantz commented 8 years ago

To reproduce this issue, clone my repo. It's my fork of a very popular ReactJS boilerplate.

Ckeckout branch v3.0.0 and run npm install.

After that run npm start and the server should start up. In the source code under app/containers/HomePage/index.js you will find that I have imported the OidcTokenManager. In the componentWillMount() lifecycle function a new OidcTokenManager is created and this throws the error.

brockallen commented 8 years ago

I'm in the middle of rewriting in ES6 -- perhaps you should look at the dev branch?

https://github.com/IdentityModel/oidc-client-js

maxmantz commented 8 years ago

@brockallen Thanks for the quick reply. I will have a look.

maxmantz commented 8 years ago

One step further - I've installed the 'oidc-client' package via npm. Then in my index.js file for Webpack (called app.js in my repo) I've added the line window.OidcClient = require('oidc-client');.

Now the first error is gone but has been replaced with the following: _oidcTokenManager2.default is not a constructor.

I've looked into the source code and I see that there is no module.exports statement which explains why nothing gets returned when I import it via require. Could I make a PR adding the statement module.exports = TokenManager to the end of the file?

brockallen commented 8 years ago

Oh no, sorry -- it's not on npm yet.

maxmantz commented 8 years ago

Ok I was able to resolve the issue by keeping a local copy of oidc-token-manager.js and in it replacing the line window.OidcTokenManager = TokenManager with module.exports = TokenManager. This will do as a quick & dirty fix until the new version is on npm. Thanks for your help.

brockallen commented 8 years ago

Yep, stay tuned for a release to npm soon. Also, feedback is very welcome on the new changes -- so some APIS will be different than docs or samples you might have seen. The ~/samples folder (right now) is the best place to see how the rewrite works.

maxmantz commented 8 years ago

@brockallen the implicit flow works with my approach using the token manager. I have created a repo - redux-oidc - which has the components to implement the flow in a redux app. It's still early in development and I would be happy to hear your thoughts about it. It contains the "hacked" version of the oidc-token-manager.js file.

Thanks again!

brockallen commented 8 years ago

Looks like you're using the current version of the library. As for your original issue, I just had a thought -- when using webpack and including a 3rd party library that creates global variables that obviously violates the "use strict" that webpack is adding. I had the same issue and had to configure my loader this way:

module: {
        loaders: [
            {
                test: /\.js$/,
                loaders: ['babel'],
                exclude: /node_modules/,
                include: __dirname,
            }
        ]
    }

Not sure exactly what in this fixes the issue, but now for the 3rd party libraries "use strict" is no longer added. I suspect it's the exclude which then doesn't transpile the 3ed party lib I was using. Maybe you could have done this originally for the npm version of my library.

maxmantz commented 8 years ago

The issue with the original library is that it doesn't export anything in a CommonJS or AMD module style. In order to use it with webpack or npm, it needs to define at least one module.exports statement, which it doesn't. I edited the export part of the library to include this. In addition, I needed to add

window.OidcClient = require('oidc-client');

at the start of the library in order for the token manager to get instantiated correctly.

My webpack config already excludes the node_modules directory which is why I never ran into the 'use strict' issues you've pointed out.

brockallen commented 8 years ago

Ah, ok... I missed the addition of the export. Yes, this is a different situation than the issue I had.

maxmantz commented 8 years ago

My repo is progressing nicely, but I ran into further issues when setting up unit tests for the project. I had to make further modifications. I've extracted the OidcClient bits into it's own file and had to mock window.localStorage by adding the following line:

window.localStorage = window.localStorage.getItem ? window.localStorage : { getItem: function(key) { return key;}};

There are further things that could be done with the file, for example externalising the CryptoJS dependency. I could set this all up by making a PR on this repo if you wish.

brockallen commented 8 years ago

Well, as I said earlier all this rework has already been done on the dev branch. I plan to release in about week, so all the work you're doing now will be different. Perhaps you should look at it? Only issue is that I don't know if or how npm supports pre-releases (in short, it's only in github now -- not on npm).

maxmantz commented 8 years ago

I guess when the release of the rewrite is only about a week away there is no need for this then. I've had a quick look at the source code and it seems like the API remains similar to the current one. I will update my dependencies as soon as it is released.

brockallen commented 8 years ago

Yes, the API very close to the existing one (semantically identical, just some cleaned up API names).

maxmantz commented 8 years ago

Just a quick heads-up: I'm getting the following error when using the token manager in Safari:

TokenManager.redirectForToken error: SYNTAX_ERR: DOM Exception 12). Are you aware of this issue and if yes will it be fixed in the ES6 rewrite of the client?

brockallen commented 8 years ago

Do you have more error info?

maxmantz commented 8 years ago

Sadly this is the only error I'm getting from Safari directly. This seems to be caused by something like this and is definitely related to this.

It seems to be an issue with Safari on Windows or older versions of the browser. I will check once the es6 rewrite is done if the problem persists.

ghost commented 7 years ago

Any developments on this?

maxmantz commented 7 years ago

Not trying to speak for @brockallen here but there is a rewrite of this library available here and I highly recommend you use it since this library is not longer maintained and fixes the issues in this thread. As for the error with Safari, I don't know about the new library. Safari for Windows uses a deprecated browser engine which caused the error. Since it was specific to Safari on Windows and the browser has almost no use in the real world, I've decided to not worry about this 👍.