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

Export types from global TypeScript file. #310

Merged
merged 8 commits into from
Jan 26, 2020

Conversation

maxswa
Copy link
Contributor

@maxswa maxswa commented Dec 17, 2019

Description

This PR is a successor to an abandoned PR: #234
It exports all typings from global.ts, allowing someone to import useful types:

import createAuth0Client, { Auth0ClientOptions } from '@auth0/auth0-spa-js';

References

Issues: #63, #39
PRs: #234

Testing

The failing integration test on the previous PR was due to the fact that it was introducing multiple exports thus making the default function createAuth0Client invalid and requires that it must be called with createAuth0Client.default

Rollup was warning about this during build:

src/index.ts → dist/auth0-spa-js.production.js...
(!) Mixing named and default exports
Consumers of your bundle will have to use bundle['default'] to access the default export, which may not be what you want. Use `output.exports: 'named'` to disable this warning

I was able to fix this by assigning the Auth0Client class to a type and exporting just the type instead of the whole class.

@maxswa maxswa requested a review from a team December 17, 2019 20:51
Copy link

@DeadHeadRussell DeadHeadRussell left a comment

Choose a reason for hiding this comment

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

This would be very helpful for our applications as it would allow us to keep strict typing with our current build system.

@stevehobbsdev
Copy link
Contributor

@maxswa Thanks for this. I'd finally penned in some time to have a look at this tonight, you beat me to raising the PR!

Looks good though, let me validate it.

@stevehobbsdev
Copy link
Contributor

stevehobbsdev commented Dec 17, 2019

This mostly looks good except I'm still unable to access Auth0Client as a type when imported into an Angular app. Changing this back to export { Auth0Client } or export { default as Auth0Client } from './Auth0Client'; allows the app to pick up the type, but then of course Rollup starts warning us about mixing named and default types.

Following this discussion, people seem divided over whether or not that mixture is bad. There's another discussion here.

I don't necessarily have a problem with this syntax (mixing default and named exports):

import createAuth0Client, { Auth0Client } from '@auth0/auth0-spa-js';

React sometimes necessitates doing the same thing when importing the React type at the same time as other things from the react module.

And it looks like having this mixture would mean that accessing the default export (createAuth0Client) in a CJS environment would be:

const createAuth0Client = require('@auth0/auth0-spa-js').default;

Since we're optimizing for the browser, I'd be ok with the compromise. Therefore I recommend we disable the warning and leave that export statement as export { Auth0Client }. I concede that it's not ideal but the alternative would be to remove the default export altogether and make it a named export, which would be a breaking change and require a new major version.

Interested in your thoughts around that.

@maxswa
Copy link
Contributor Author

maxswa commented Dec 18, 2019

This mostly looks good except I'm still unable to access Auth0Client as a type when imported into an Angular app. Changing this back to export { Auth0Client } or export { default as Auth0Client } from './Auth0Client'; allows the app to pick up the type, but then of course Rollup starts warning us about mixing named and default types.

...

I also don't really see an issue with mixing default and named exports. As you have said, I end up using the pattern a lot when working with react.

const createAuth0Client = require('@auth0/auth0-spa-js').default; is a bit ugly but it's not worth making breaking changes over in my opinion.
I'd guess that the majority of people using this package are building apps using ESM anyway.

@stevehobbsdev stevehobbsdev requested review from stevehobbsdev and removed request for a team December 18, 2019 10:21
Copy link
Contributor

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

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

@damieng Is there anything we can do here with this PR that won't involve a breaking change, and also allow us to have exported types?

We're at a stage where we can either:

  1. Export all types except Auth0Client (a core type for this library) and have everything work except being able to import the Auth0Client type (not an option IMO)
  2. Export everything as named types and break the import of createAuth0Client for CJS users and those pulling it in from the CDN.

The issue is we're trying to export a named type and a default type, which won't work for CSJ builds as the default export createAuth0Client gets wrapped in an object with a default property.

Another change could be to export createAuth0Client as a named export but this again is a breaking change.

static/index.html Outdated Show resolved Hide resolved
@maxswa
Copy link
Contributor Author

maxswa commented Dec 18, 2019

Actually there is a way to solve this. If we export Auth0Client as a type we can keep the old rollup config and prevent the breaking change.
The only problem is that TS will yell at us for conflicting declarations. So we can either suppress the warning or name the type something different:

// @ts-ignore
import Auth0Client from './Auth0Client';

// ...

export type Auth0Client = Auth0Client;

vs

import Auth0Client from './Auth0Client';

// ...

export type Auth0ClientType = Auth0Client;

Thoughts?

@goszczynskip
Copy link

What about creating index.d.ts manually with exported types. It can be used to generate artefacts only. Downsides - it complicates build process.
Final src structure would look like this:

...
index.ts - current entry, without re-exported types
index.d.ts - new entry with re-exported types
...

index.d.ts can be compiled with tsc that will generate only ts artefacts.
Generating artefacts in current config needs to be disabled.

@stevehobbsdev
Copy link
Contributor

@goszczynskip Exporting the type and using @ts-ignore looks like the best option from here, rather than complicating the build process. But it's a neat idea!

Let's go with this:

// @ts-ignore
import Auth0Client from './Auth0Client';

// ...

export type Auth0Client = Auth0Client;

I've just done a bit of testing with it and at first pass it appears to get what we want.

@maxswa can you:

  • Revert the configuration for rollup and remove the named exports config
  • Implement the change above
  • Change index.html so that it uses createAuth0Client instead of createAuth0Client.default

I think that should stand us in good stead. I'd also be happy to hear from anyone who is willing to validate these changes in an app that they have which is waiting for these type changes to come in. The more eyes we can get on this the better.

import Auth0Client from './Auth0Client';
import * as ClientStorage from './storage';

//this is necessary to export the type definitions used in this file
import { Auth0ClientOptions } from './global';
import './global';

Choose a reason for hiding this comment

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

Is there any reason why it's left there? It looks like you can delete this line.

Copy link
Contributor Author

@maxswa maxswa Dec 22, 2019

Choose a reason for hiding this comment

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

This is to prevent breaking changes for people that are using the types when they are exposed globally by

import createAuth0Client from '@auth0/auth0-spa-js';

Edit: See comment below

Copy link
Contributor Author

@maxswa maxswa Dec 31, 2019

Choose a reason for hiding this comment

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

Actually after looking into this more, it looks like it actually is introducing breaking changes as the types are no longer ambiently declared after being exported. I'm looking into a fix for this now

Copy link
Contributor

Choose a reason for hiding this comment

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

@maxswa What is the best way to reproduce the failure with the type export that you're seeing?

Copy link
Contributor

Choose a reason for hiding this comment

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

@maxswa Did you manage to look into this?

Copy link
Contributor Author

@maxswa maxswa Jan 17, 2020

Choose a reason for hiding this comment

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

If I import createAuth0Client from '@auth0/auth0-spa-js'; with my local version of the lib, I cannot use the Auth0 TypeScript types without explicitly importing them. I have not been able to find a solution that lets us export all of the types while also ambiently declaring them. Would this be an acceptable breaking change for this library? It would only effect TypeScript projects and the migration would be very straightforward.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maxswa I'm not sure that will at least make it any worse than it is now. Let me carve out some time on Monday to play with this but I don't think the types can be picked up ambiently at the moment anyway. Right now people have to import the types, but doing so by importing the specific type definition file rather than just importing from @auth0/auth0-spa-js.

Copy link
Contributor

@stevehobbsdev stevehobbsdev Jan 17, 2020

Choose a reason for hiding this comment

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

I'm playing with it now and the only problem I can see so far is that VS Code wants to auto-resolve the Auth0Client type to @auth0/auth0-spa-js/dist/typings/Auth0Client, but if I manually import it from the correct package, i.e.:

import createAuth0Client, { Auth0Client } from "@auth0/auth0-spa-js";

It seems to work fine.

@stevehobbsdev stevehobbsdev added this to the vNext milestone Jan 17, 2020
@stevehobbsdev stevehobbsdev added CH: Added PR is adding feature or functionality review:small labels Jan 17, 2020
src/global.ts Show resolved Hide resolved
@stevehobbsdev
Copy link
Contributor

We've had a discussion internally about this and we're going to bring it in on the beta channel. Thanks @maxswa for the contribution and helping to get this over the line!

@stevehobbsdev stevehobbsdev removed this from the vNext milestone Jan 24, 2020
@stevehobbsdev stevehobbsdev added this to the v1.7.0-beta.3 milestone Jan 24, 2020
@maxswa
Copy link
Contributor Author

maxswa commented Jan 24, 2020

We've had a discussion internally about this and we're going to bring it in on the beta channel. Thanks @maxswa for the contribution and helping to get this over the line!

No problem!

@stevehobbsdev stevehobbsdev changed the base branch from master to release/rtr January 26, 2020 22:24
@stevehobbsdev stevehobbsdev merged commit 02ecd83 into auth0:release/rtr Jan 26, 2020
@stevehobbsdev
Copy link
Contributor

This was merged and the build subsequently fixed (had a couple of conflicts on the types since merging into the beta branch that also had some types changes).

Look out for this coming in 1.7.0-beta.3. Thanks again, @maxswa!

@bgilm
Copy link

bgilm commented Apr 16, 2020

So this is my working Auth0Provider, I'll leave it here as a reference for others looking on how to use this new exports.

import React, { useState, useEffect, useContext } from "react";
import createAuth0Client, {
  Auth0Client,
  PopupLoginOptions,
  RedirectLoginResult,
  getIdTokenClaimsOptions,
  IdToken,
  RedirectLoginOptions,
  GetTokenSilentlyOptions,
  GetTokenWithPopupOptions,
  LogoutOptions,
  Auth0ClientOptions,
} from "@auth0/auth0-spa-js";

interface IAuth0Context {
  isAuthenticated: boolean;
  user: any;
  loading: boolean;
  popupOpen: boolean;
  loginWithPopup(options: PopupLoginOptions): Promise<void>;
  handleRedirectCallback(): Promise<RedirectLoginResult>;
  getIdTokenClaims(o?: getIdTokenClaimsOptions): Promise<IdToken>;
  loginWithRedirect(o: RedirectLoginOptions): Promise<void>;
  getTokenSilently(o?: GetTokenSilentlyOptions): Promise<string | undefined>;
  getTokenWithPopup(o?: GetTokenWithPopupOptions): Promise<string | undefined>;
  logout(o?: LogoutOptions): void;
}
interface IAuth0ProviderOptions {
  children: React.ReactElement;
  onRedirectCallback?(
    result: RedirectLoginResult | { targetUrl: string | null | undefined }
  ): void;
}

const DEFAULT_REDIRECT_CALLBACK = () =>
  window.history.replaceState({}, document.title, window.location.pathname);

export const Auth0Context = React.createContext<IAuth0Context | null>(null);
export const useAuth0 = () => useContext(Auth0Context)!;
export const Auth0Provider = ({
  children,
  onRedirectCallback = DEFAULT_REDIRECT_CALLBACK,
  ...initOptions
}: IAuth0ProviderOptions & Auth0ClientOptions) => {
  const [isAuthenticated, setIsAuthenticated] = useState(false);
  const [user, setUser] = useState();
  const [auth0Client, setAuth0] = useState<Auth0Client>();
  const [loading, setLoading] = useState(true);
  const [popupOpen, setPopupOpen] = useState(false);

  useEffect(() => {
    const initAuth0 = async () => {
      const auth0FromHook = await createAuth0Client(initOptions);
      setAuth0(auth0FromHook);

      if (window.location.search.includes("code=")) {
        const { appState } = await auth0FromHook.handleRedirectCallback();
        onRedirectCallback(appState);
      }

      const isAuthenticated = await auth0FromHook.isAuthenticated();

      setIsAuthenticated(isAuthenticated);

      if (isAuthenticated) {
        const user = await auth0FromHook.getUser();
        setUser(user);
      }

      setLoading(false);
    };
    initAuth0();
    // eslint-disable-next-line
  }, []);

  const loginWithPopup = async (o: PopupLoginOptions) => {
    setPopupOpen(true);
    try {
      await auth0Client!.loginWithPopup(o);
    } catch (error) {
      console.error(error);
    } finally {
      setPopupOpen(false);
    }
    const user = await auth0Client!.getUser();
    setUser(user);
    setIsAuthenticated(true);
  };

  const handleRedirectCallback = async () => {
    setLoading(true);
    const result = await auth0Client!.handleRedirectCallback();
    const user = await auth0Client!.getUser();
    setLoading(false);
    setIsAuthenticated(true);
    setUser(user);
    return result;
  };
  return (
    <Auth0Context.Provider
      value={{
        isAuthenticated,
        user,
        loading,
        popupOpen,
        loginWithPopup,
        handleRedirectCallback,
        getIdTokenClaims: (o: getIdTokenClaimsOptions | undefined) =>
          auth0Client!.getIdTokenClaims(o),
        loginWithRedirect: (o: RedirectLoginOptions) =>
          auth0Client!.loginWithRedirect(o),
        getTokenSilently: (o: GetTokenSilentlyOptions | undefined) =>
          auth0Client!.getTokenSilently(o),
        getTokenWithPopup: (o: GetTokenWithPopupOptions | undefined) =>
          auth0Client!.getTokenWithPopup(o),
        logout: (o: LogoutOptions | undefined) => auth0Client!.logout(o),
      }}
    >
      {children}
    </Auth0Context.Provider>
  );
};

helloworld12321 added a commit to UMM-CSci-3601-S20/it-3-007 that referenced this pull request Apr 21, 2020
This class existed before, but its implementation was minimal—it didn't actually mock anything. Now, most methods and properties are overridden with fake versions.

(Actually, we're not really mocking AuthService itself; we're mostly mocking Auth0Client, which AuthService is a thin wrapper around.)

To do this, we had to bump `@auth0/auth0-spa-js` from 1.6.5 to 1.7.0, because previously Auth0's TypeScript support was iffy. (See auth0/auth0-spa-js#310)
helloworld12321 added a commit to UMM-CSci-3601-S20/it-3-007 that referenced this pull request Apr 21, 2020
This class existed before, but its implementation was minimal—it didn't actually mock anything. Now, most methods and properties are overridden with fake versions.

(Actually, we're not really mocking AuthService itself; we're mostly mocking Auth0Client, which AuthService is a thin wrapper around.)

To do this, we had to bump `@auth0/auth0-spa-js` from 1.6.5 to 1.7.0, because previously Auth0's TypeScript support was iffy. (See auth0/auth0-spa-js#310)
helloworld12321 added a commit to UMM-CSci-3601-S20/it-3-007 that referenced this pull request Apr 30, 2020
In particular, this commit gets us the latest version of auth0-spa-js,
which we need for testing. (See auth0/auth0-spa-js#310)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Added PR is adding feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants