-
-
Notifications
You must be signed in to change notification settings - Fork 29
Conversation
🎉 This PR is included in version 1.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@@ -62,6 +64,9 @@ | |||
"husky": "^7.0.4", | |||
"lint-staged": "^12.1.2", | |||
"next": "^12.0.7", | |||
"nextv9": "npm:[email protected]", | |||
"nextv10": "npm:[email protected]", | |||
"nextv11": "npm:[email protected]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RyanClementsHax I know there was a minor version that changed the location of RouterContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. It was 11.1 that had those changes. This package is a dev dependency just for typings though. Do you think I should also include 11.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RyanClementsHax depends, you could probably just get away with telling them to upgrade but it'd also be a more seamless experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point
|
||
try { | ||
// eslint-disable-next-line @typescript-eslint/no-var-requires | ||
const NextImage = require('next/image') as typeof _NextImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RyanClementsHax pretty sure there's a better way here, e.g. you define the code for the stub here, but then in webpack you check which version via package.json the next package that is installed is and load it only when its > 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I considered this originally but didn't see an obvious way to tackle this via webpack. Do you see a way?
// this will be aliased by webpack at runtime (this is just for typing) | ||
import { RouterContext } from './resolved-router-context' | ||
// this will be aliased by webpack at runtime (this is just for typing) | ||
import Router from './resolved-router' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RyanClementsHax might be good to just alias whatever the latest nextjs path is and patch all previous versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is definitely a way forward. I however don't have any previous versions I can use for that since I made the addon a week or so ago and the first version only included next v12 support. Do u see a way around this?
Please forgive how large this PR is. It includes not only the code changes (which are only moderately sized), but also the example projects to go along with them (lots of boiler plate files)