request for feedback: change classes to use named exports instead of default exports

[DanielSchaffer]

I'm a TypeScript user, and I've been attempting to switch over to using ES modules where possible to enable tree shaking in Webpack. While oidc-client-js is written using ES6 modules (yay!), all the classes use default exports, which wouldn't be a problem, except that they are then re-exported from the index in a way that doesn't match up with what's in the type definition file. The type definition is written as though all the classes are exported using named exports (e.g. export class UserManager), whereas the index exports default export that is a single object with properties for each of the classes.

I've submitted a PR (#508), which fixes the problem (without affecting UMD users) by switching the classes over to using named exports, but I haven't received any feedback yet. Is there anything I can do to help expedite the PR?

[brockallen]

Sorry -- I have been busy and am traveling. I won't have time to get back to the open issues/PRs here for a couple of weeks.

[brockallen]

That's a breaking change for users of module loaders, yes?

[DanielSchaffer]

Sorry for the delay, I was doing some traveling of my own!

This should not be a breaking change for anyone - it should be transparent for anyone using the existing UMD modules. I tried it with webpack, and didn't have any issues (other than needing { node: { fs: 'empty' } } to make jsrsasign happy). Do you have any test apps I can use to confirm?

[brockallen]

The only one I have is here: https://github.com/IdentityServer/IdentityServer4.Samples/tree/release/Clients/src/JsOidc

[DanielSchaffer]

Shoot, I don't have any netcore stuff set up :\ Do you have a way to run it quickly and just swap out the js files? https://github.com/DanielSchaffer/oidc-client-js/tree/dev/dist

[brockallen]

Sorry, I'll keep the thread here...

So this is a breaking change for folks using this as ES6 module? If so, I might just bump the next version to 2.0.

[brockallen]

I merged (manually) this into the dev branch. Have a look please and let me know if it's working the way you need.

[DanielSchaffer]

Ahhhh yes. For ES6 - it depends, I think. I wasn't ever able to figure out how to import modules from the index correctly - I'd end up with some sort of ES module object, and would've had to call .default on the import in order to actually use the class - so I'd ended up importing from the source files directly - and if anyone else was doing that, then yes, I believe this would be a breaking change, since those use named exports instead of defaults now.

I'll take a look at dev tonight. Thank you!

[DanielSchaffer]

Had a few mins to check into this, and I'm running into a few issues. First, there's a typo in index.d.ts (SignoutRespsone instead of SignoutResponse), which is easy enough to fix. But I'm also having issues with the babel plugins somehow sneaking into my build process, which is conflicting with the stuff I've got set up there. I think this might just be due to trying to test it out using npm link instead of as an actual installed dependency, which wouldn't respect the .npmignore you've got set up. I'll have to poke around a bit more tonight.

[brockallen]

yea, odd. from what i recall the polyfill is onlt being pulled in when we build what goes into dist or lib. if you just import is ES6 style, then i don't know why.

[brockallen]

Also, .ts typo is fixed

[DanielSchaffer]

Okay, I can confirm that it's just the .babelrc file being there when using npm link. It works fine when I remove it. Can you cut another prerelease (with the typo fix) so I can confirm with the actual package?

[brockallen]

I merged in a fix for that. I just released 1.5.0-beta.2. Let me know.

[brockallen]

Any update on this? If it's working, I'd like to include it for you (and I have time to work on this today).

[brockallen]

Ok, I'll close this since I think it's what you're asking for,

[DanielSchaffer]

Shoot, sorry I hadn't updated. I ran into issues with my build chain on this, but it appears to just be due to including that jsrsasign library in the source, which I can work around. That said, that should really be a dependency - is the owner not responding to PRs? Why not fork it?

[brockallen]

Their build system isn't really in their repo and so it's hard to repro the steps they take. Also, their version of the minimal build is actually not what is needed by this library to validate JWTs. I wish they were more receptive, but ... well, I took this step as the past of least resistance to get a minimal sized version of their code.