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

fix: use package ranges instead of specifying versions #292

Closed
wants to merge 1 commit into from

Conversation

eligundry
Copy link
Contributor

@eligundry eligundry commented Jan 6, 2023

This undos the exact version change pushed in #282

  • remix-auth is moved to peerDependencies as the documentation for remix-auth-spotify specifies that it should be manually installed.
  • remix-auth is added to devDependencies for any tests that rely on remix have access to them.
  • remix-auth-oauth2 is made into a package range that will support all 1.x.x releases >= 1.5.0.

This change was made because installing as specifed in the documentation lead to multiple versions of remix being installed which does not work with any typescript type overloading that you might do.

remix-run/remix#1876

For example, I have an existing remix app using [email protected] with the following in my remix.env.d.ts:

// in remix.env.d.ts
import '@remix-run/server-runtime'

declare module '@remix-run/server-runtime' {
  interface AppLoadContext {
    requestId: string
  }
}

This allows me to access the requestId in a loader like so:

import { LoaderArgs } from '@remix-run/node'

export async function loader({ context }: LoaderArgs) {
  foobar(context.requestId)
}

Installing remix-auth-spotify as specified in the docs caused [email protected] to be installed alongside 1.7.4. This caused the type overriding in Typescript to not work at all and make context.requestId become unknown. This was resolved by bumping up remix to 1.9.0 in my package.json. By using package ranges, downstream consumers of this package are able to define the versions of remix.

Question: If accepted, would it make sense to also have remix-auth-oauth2 moved to peerDependencies? I could envision an authentication situation where the upstream consumer of this package has Spotify auth AND a custom OAuth2 setup. I don't have a super strong stance, though.

This undos the exact version change pushed in JosteinKringlen#282

- remix-auth is moved to `peerDependencies` as the documentation for
  remix-auth-spotify specifies that it should be manually installed.
- remix-auth is added to `devDependencies` for any tests that rely on
  remix have access to them.
- remix-auth-oauth2 is made into a package range that will support all
  1.x.x releases >= 1.5.0.

This change was made because installing as specifed in the documentation
lead to multiple versions of remix being installed which does not work
with any typescript type overloading that you might do.

remix-run/remix#1876

For example, I have an existing remix app using [email protected] with the
following in my remix.env.d.ts:

```typescript
// in remix.env.d.ts
import '@remix-run/server-runtime'

declare module '@remix-run/server-runtime' {
  interface AppLoadContext {
    requestId: string
  }
}
```

This allows me to access the `requestId` in a `loader` like so:

```typescript
import { LoaderArgs  } from '@remix-run/node'

export async function loader({ context }) {
  foobar(context.requestId)
}
```

Installing remix-auth-spotify as specified in the docs caused
[email protected] to be installed alongside 1.7.4. This caused the type
overriding in Typescript to not work at all and make `context.requestId`
become unknown. This was resolved by bumping up remix to 1.9.0 in my
package.json. By using package ranges, downstream consumers of this
package are able to define the versions of remix.

Question: If accepted, would it make sense to also have
remix-auth-oauth2 moved to `peerDependencies`? I could envision an
authentication a situation where the upstream consumer of this package
has Spotify auth AND a custom OAuth2 setup. I don't have a super strong
stance, though.
Copy link
Owner

@JosteinKringlen JosteinKringlen left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the late review! Makes sense to move the dependencies around a bit - good thinking! :)

When it comes to moving remix-auth-oauth2 to peerDependencies; I guess it could make sense to have it as a peer dep, though I haven't really given it too much thought :) It's for sure something to consider if there's any interest/need for it :)

@eligundry
Copy link
Contributor Author

@JosteinKringlen thank you so much for implementing this (and going out of your way to add me as a co-author!). Closing as this looks good now!

@eligundry eligundry closed this Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants