-
Notifications
You must be signed in to change notification settings - Fork 394
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 Windows development #1205
Fix Windows development #1205
Conversation
@jorgeorpinel ping? |
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.
Oh I already included this in my PR over here: #1210. It does work indeed, thanks again @rogermparent!
This PR is now confirmed to fix, and its commits are included in #1210 |
@@ -78,7 +78,8 @@ | |||
"scroll": "^3.0.1", | |||
"serve-handler": "^6.1.2", | |||
"slick-carousel": "^1.8.1", | |||
"unist-util-visit": "2.0.2" | |||
"unist-util-visit": "2.0.2", | |||
"upath": "^1.2.0" |
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.
not sure ... but should it be part of the devDeps section?
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.
I suppose it would fit more in the current setup, but technically this is the right place for it and most of those devDependencies should be moved up to regular dependencies. It makes no difference in the practical sense as the switch to YARN_PRODUCTION=false
means we keep all deps and devDeps in the same way on deploy.
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.
yep, you are right. For the website there are no difference at all, right?
may be we can keep linters, tests, and other stuff there? may be should move them to optionalDeps? to avoid installing them on Heroku?
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.
anyway, merging this ... since this one can stay in deps.
No need to rebase Roger. I properly merged your branch to mine so Git will know what needs to be merged where/when. |
Ever since Models dropped, we use
path.join
to resolve links between files at build time (e.g. Blog Posts and Authors). I believe this is what's breaking Windows development, so I used a small module calledupath
that wraps the path API with cross-platform safe versions of functions likejoin
.upath
is already installed by another module as you can see inyarn.lock
, but the other package specifies an earlier version. The minimal size difference at build time and completely unchanged bundle size make it not worth pinning our usage to the same version, in my opinion.Fixes #1204, or at least I think it does. It will before merging, at least.