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.

Sign up for GitHub

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

Add support for Windows NPM #658

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Add support for Windows NPM #658

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6148029
add support for windows NPM
MetaMmodern Jun 29, 2022
8fb15a2
lint: remove redundant import
MetaMmodern Jun 29, 2022
18d5cca
Fixed path in generated JS file that was not always Posix.
Martinsos Jun 30, 2022
496d1b3
Now we decode output of subprocesses with locale encoding instead of …
Martinsos Jun 30, 2022
d9b3c4a
fix: types to be Strings
MetaMmodern Jul 2, 2022
53f14df
lint: clear the namespace
MetaMmodern Jul 2, 2022
bea2d07
fix: bad naming
MetaMmodern Jul 2, 2022
14001b5
add function comments
MetaMmodern Jul 2, 2022
2ddeb9f
format: with ormulu
MetaMmodern Jul 2, 2022
e9a4f24
Update waspc/src/Wasp/Generator/Common.hs
MetaMmodern Jul 2, 2022
bdafb28
fix: change call signature to npm only
MetaMmodern Jul 2, 2022
b7316d2
Merge branch 'main' of https://github.com/MetaMmodern/wasp
MetaMmodern Jul 2, 2022
8f7d68d
fix: comments placement
MetaMmodern Jul 2, 2022
9b531fb
Fixed compiler errors, refactored node job function a bit.
Martinsos Jul 4, 2022
09a6a51
Renamed node-cmd-as-job function.
Martinsos Jul 4, 2022
9115d5e
Added additional encodings support to Wasp.
Martinsos Jul 4, 2022
061af90
Modified encodings flag.
Martinsos Jul 4, 2022
b18bfb5
Fixed more for Windows
Martinsos Jul 4, 2022
f0a3771
Finally fixed encodings?
Martinsos Jul 5, 2022
ed900ae
Implemented encoding function for ByteString.
Martinsos Jul 7, 2022
6d8c376
Removed knob library and now using direct decoding.
Martinsos Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions waspc/src/Wasp/Generator/Common.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@ module Wasp.Generator.Common
nodeVersionRange,
npmVersionRange,
prismaVersion,
oSSpecificNpm,
compileOsSpecificNodeCommand
)
where

import qualified Wasp.SemanticVersion as SV
import System.Info (os)
import Data.List
Copy link
Member

Choose a reason for hiding this comment

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

We try to import stuff qualified or named, so we don't pollute the namespace.
Therefore, best to do import Data.List (intercalate)!

Copy link
Author

Choose a reason for hiding this comment

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

Done


-- | Directory where the whole web app project (client, server, ...) is generated.
data ProjectRootDir
Expand Down Expand Up @@ -38,3 +42,12 @@ npmVersionRange =

prismaVersion :: SV.Version
prismaVersion = SV.Version 3 15 2

oSSpecificNpm :: [Char]
Copy link
Member

Choose a reason for hiding this comment

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

Nice job, you put this in the best place in the codebase I think :), finding your way around well I see!

Maybe we can just call this function npmCmd? The fact that it is OS specific we can leave as an implementation detail of the function itself.

A bit of Haskell: instead of [Char], normally we use String -> it is just type alias for [Char] really, but it is more common.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could also provide a short comment explaining why is it important to add this .cmd on windows, since it took us some time to figure that out, and if somebody later stumbles onto this, they might wonder why we are adding this .cmd on windows.

Copy link
Author

Choose a reason for hiding this comment

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

@Martinsos for Strings--got it, did not think haskell has those) For exmplanation comment--no problem. Should I leave a text explanation or leave a link to SO?

Copy link
Member

@Martinsos Martinsos Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe best to do both: text explanation + SO link! Your explanations here in description are already very good, smth very similar should do just fine.

oSSpecificNpm = "npm" ++ if os /= "mingw32" then "" else ".cmd"

compileOsSpecificNodeCommand :: [Char] -> [[Char]] -> ([Char], [[Char]])
Copy link
Member

Choose a reason for hiding this comment

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

We could maybe also call this one just compileNodeCmdWithArgs, and leave the fact about OS as internal thing.

s/[Char]/String

Here we could also benefit from a comment explaining the reasoning behind the stuff we have to do for Windows. In this case it would also explain it to me since right now I don't understand why we have to do this :D! I thought node was working fine for us?

EDIT: reading code below, I realized this command here and args are not node + some args, it is instead ANY command that we somehow know requires node to be installed on the machine + its args. I suggested we rename on of the old functions below, check it out -> I gave it poor name before and I confused me now, possibly you also. So if we rename that one, we should also rename this one to compileCmdWithArgsRequiringNode.

But most important will be documenting what is the intention of this function, then further direction will be clearer.

compileOsSpecificNodeCommand command arguments =
if os /= "mingw32"
then (command, arguments)
else ("cmd.exe", [intercalate " " (["/c", command] ++ arguments)])
4 changes: 2 additions & 2 deletions waspc/src/Wasp/Generator/DbGenerator/Jobs.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import StrongPath (Abs, Dir, Path', (</>))
import qualified StrongPath as SP
import System.Exit (ExitCode (..))
import qualified System.Info
import Wasp.Generator.Common (ProjectRootDir, prismaVersion)
import Wasp.Generator.Common (ProjectRootDir, prismaVersion, oSSpecificNpm)
import Wasp.Generator.DbGenerator.Common (dbSchemaFileInProjectRootDir)
import Wasp.Generator.Job (JobMessage, JobMessageData (JobExit, JobOutput))
import qualified Wasp.Generator.Job as J
Expand Down Expand Up @@ -80,7 +80,7 @@ generatePrismaClient projectDir = do
npmInstall :: Path' Abs (Dir ProjectRootDir) -> J.Job
npmInstall projectDir = do
let serverDir = projectDir </> serverRootDirInProjectRootDir
runNodeCommandAsJob serverDir "npm" ["install"] J.Db
runNodeCommandAsJob serverDir oSSpecificNpm ["install"] J.Db

data JobMessageForwardingStrategy = ForwardEverything | ForwardOnlyRetryErrors

Expand Down
3 changes: 2 additions & 1 deletion waspc/src/Wasp/Generator/Job/Process.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ runNodeCommandAsJob fromDir command args jobType chan = do
Right nodeVersion ->
if SV.isVersionInRange nodeVersion C.nodeVersionRange
then do
let process = (P.proc command args) {P.cwd = Just $ SP.fromAbsDir fromDir}
let (specificCommand, specificArgs) = C.compileOsSpecificNodeCommand command args
Copy link
Member

Choose a reason for hiding this comment

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

Looking at runNodeCommandAsJob, I am thinking that it is not clear what it actually does. This falls on me, I should have documented it better! But reading it now, I think the point is that it runs any command with args, while first checking if node is available on the machine and is using correct version?

So, it should probably be renamed to runCommandThatRequiresNodeAsJob! Maybe we can do this in this PR since we are already here?

Copy link
Member

Choose a reason for hiding this comment

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

As for using compileOsSpecificNodeCommand here -> We are basically applying it to any command really, not node command, just any command that for some reason requires node to be present on the machine. Is that ok, is that what we wanted? I guess I don't know because I am not sure what that cmd.exe does and why we are doing it.

If so, should we use it for all commands we are trying to run, and not just here?

What about us calling node directly, I see that below we are calling node, in line 129 -> what about that, is that ok to be left like that?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so now I read what you wrote about cmd.exe and read the SO answer you linked to!
So interesting, it seems that npm.cmd script is written in such way that calling it programmatically can make it work wrong, so by adding cmd.exe /c in front we ensure it works correctly.

The question is then: Do we want to use this trick only when calling npm, or do we want to use it when calling any commands on Windows? I am thinking maybe it is enough to use it just for calling npm, at least for now?

In that case, let's use it like that, only for npm. So what we could do is rename this function to

buildNpmCmdWithArgs :: [String] -> (String, [String])

and that is it? And then we call it only in places where we need to call npm?

let process = (P.proc specificCommand specificArgs) {P.cwd = Just $ SP.fromAbsDir fromDir}
runProcessAsJob process jobType chan
else
exitWithError
Expand Down
4 changes: 2 additions & 2 deletions waspc/src/Wasp/Generator/ServerGenerator/Setup.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ module Wasp.Generator.ServerGenerator.Setup
where

import StrongPath (Abs, Dir, Path', (</>))
import Wasp.Generator.Common (ProjectRootDir)
import Wasp.Generator.Common (ProjectRootDir, oSSpecificNpm)
import qualified Wasp.Generator.Job as J
import Wasp.Generator.Job.Process (runNodeCommandAsJob)
import qualified Wasp.Generator.ServerGenerator.Common as Common

installNpmDependencies :: Path' Abs (Dir ProjectRootDir) -> J.Job
installNpmDependencies projectDir = do
let serverDir = projectDir </> Common.serverRootDirInProjectRootDir
runNodeCommandAsJob serverDir "npm" ["install"] J.Server
runNodeCommandAsJob serverDir oSSpecificNpm ["install"] J.Server
4 changes: 2 additions & 2 deletions waspc/src/Wasp/Generator/ServerGenerator/Start.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ module Wasp.Generator.ServerGenerator.Start
where

import StrongPath (Abs, Dir, Path', (</>))
import Wasp.Generator.Common (ProjectRootDir)
import Wasp.Generator.Common (ProjectRootDir, oSSpecificNpm)
import qualified Wasp.Generator.Job as J
import Wasp.Generator.Job.Process (runNodeCommandAsJob)
import qualified Wasp.Generator.ServerGenerator.Common as Common

startServer :: Path' Abs (Dir ProjectRootDir) -> J.Job
startServer projectDir = do
let serverDir = projectDir </> Common.serverRootDirInProjectRootDir
runNodeCommandAsJob serverDir "npm" ["start"] J.Server
runNodeCommandAsJob serverDir oSSpecificNpm ["start"] J.Server
4 changes: 2 additions & 2 deletions waspc/src/Wasp/Generator/WebAppGenerator/Setup.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ module Wasp.Generator.WebAppGenerator.Setup
where

import StrongPath (Abs, Dir, Path', (</>))
import Wasp.Generator.Common (ProjectRootDir)
import Wasp.Generator.Common (ProjectRootDir, oSSpecificNpm)
import qualified Wasp.Generator.Job as J
import Wasp.Generator.Job.Process (runNodeCommandAsJob)
import qualified Wasp.Generator.WebAppGenerator.Common as Common

installNpmDependencies :: Path' Abs (Dir ProjectRootDir) -> J.Job
installNpmDependencies projectDir = do
let webAppDir = projectDir </> Common.webAppRootDirInProjectRootDir
runNodeCommandAsJob webAppDir "npm" ["install"] J.WebApp
runNodeCommandAsJob webAppDir oSSpecificNpm ["install"] J.WebApp
4 changes: 2 additions & 2 deletions waspc/src/Wasp/Generator/WebAppGenerator/Start.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ module Wasp.Generator.WebAppGenerator.Start
where

import StrongPath (Abs, Dir, Path', (</>))
import Wasp.Generator.Common (ProjectRootDir)
import Wasp.Generator.Common (ProjectRootDir, oSSpecificNpm)
import qualified Wasp.Generator.Job as J
import Wasp.Generator.Job.Process (runNodeCommandAsJob)
import qualified Wasp.Generator.WebAppGenerator.Common as Common

startWebApp :: Path' Abs (Dir ProjectRootDir) -> J.Job
startWebApp projectDir = do
let webAppDir = projectDir </> Common.webAppRootDirInProjectRootDir
runNodeCommandAsJob webAppDir "npm" ["start"] J.WebApp
runNodeCommandAsJob webAppDir oSSpecificNpm ["start"] J.WebApp