Windows Local Dev: Construct build scripts for cross-platform use

[thedavidprice]

Closes #352

Some package.json scripts failed on a Windows system and blocked the ability to contribute.

• setting NODE_ENV=production before a command fails on Windows
• Linux commands such as mv, cp, and rm are not cross-platform

env vars

This PR uses cross-env to set env vars across all packages:
https://www.npmjs.com/package/cross-env

I used yarn cross-env ... instead of just cross-env ..., which seems to improve readability.

removing dist/ and moving non-js files with babel

Now using babel --delete-dir-on-start instead of rm -rf /dist, which allowed me to remove yarn clean.

And babel --copy-files supports moving non-js files into the dist/ directory.

API package specific changes

Because the api/ package requires moving importAll.macro.js outside of dist/ during a build (and then also deleting it prior to a build), I used two packages for cross-platform support of the commands rm and mv.

• https://www.npmjs.com/package/move-cli
• https://www.npmjs.com/package/del-cli

It feels a bit messy...

This works, but it seems like a workaround instead of a proper process.

@peterp I suggest we discuss:

• leaving the file inside of dist/, which means a breaking change in CRA import for api/src/functions/graphql.js
• exploring your idea for converting the file to a proper babel macro via: https://github.com/kentcdodds/babel-plugin-macros

[mohsen1]

Love it! Let's run the build scripts and tests on Windows in GitHub actions as well. Here is the docs

[thedavidprice]

@mohsen1 Interesting suggestion (and I ❤️GHActions), but for our publishing workflow, I'm not sure this is needed. Or more specifically, since the heart of the problem on Windows is actually Powershell support vs Git Bash vs Etc., I don't think this would actually provide a helpful test -- we'd end up with false positives, effectively.

Talk me into it?

[mohsen1]

I don't have a lot of experience with Windows in GitHub actions. I think it's worth looking into. I think the default is PowerShell now. Things like path separator for generated files and end of file/line chars can be checked in Windows.

You can add windows to the build matrix and let it run. if we saw new issues reported by Windows users we can see how we can test against that in CI then.

I don't have Windows to try things out.

[peterp]

exploring your idea for converting the file to a proper babel macro via: https://github.com/kentcdodds/babel-plugin-macros

I think this is the best approach, but I would love to do that in the next release.

[peterp]

Not a train smash, but I have trouble mentally parsing --delete-dir-on-start src since it looks like that is the directory to delete, but it's actually the files to compile (I know they're essentially the same.)

Suggested change

	"build": "yarn cross-env NODE_ENV=production babel --delete-dir-on-start src --out-dir dist --copy-files --no-copy-ignored",
	"build": "babel --out-dir dist --env-name PRODUCTION --delete-dir-on-start --copy-files --no-copy-ignored src",

I also found the --env-name flag in babel-cli!

[peterp]

  --env-name [name]                           The name of the 'env' to use when loading configs and plugins. Defaults to the value of
                                              BABEL_ENV, or else NODE_ENV, or else 'development'.

[thedavidprice]

Agreed these are painful to read. Feel the same and probably spent too much on that myself before getting reactions. Anyway, for both of these you’re suggesting I try:

1. re-ordering flags to make --delete-dir-on-start more understandable. I could try:
   yarn cross-env NODE_ENV=production babel src --out-dir dist --delete-dir-on-start --copy-files --no-copy-ignored
2. See if --env-name flag can replace cross-env package implementation

Correct?

[peterp]

Yup! I actually prefer your suggestion to add src to the front!

[thedavidprice]

Babel Docs are just, grrrr.

--env-name does not allow us to set the variable, apparently. It’s just for choosing an env preset, which doesn’t really work with our current setup. But in the future this could be handy — could have settings specific to BABLE_ENV separate from those for NODE_ENV. Anyway, I actually found in some earlier version of the Babel docs where they just say to "use cross-env”. There you have it.

[thedavidprice]

@peterp any chance you could run this as-in within your Windows env and see if it even works? I can do the same if no time now. Will just take me a bit longer.

[peterp]

I've made some suggestions, there's a --env-name flag that I just saw, where we can maybe remove cross-env?

I also mentioned something about a --watch flag where we might be able to remove nodemon?

[thedavidprice]

@peterp See my comments about, well, babel == grrr. I did improve readability per comments.

Going to take a quick stab at GH Action matrix build including Windows. BRB

[thedavidprice]

@peterp

Matrix Build: Ubuntu, Windows, Mac OS
For testing, this branch is now running tests in parallel across these three OS's and Node 12. Runtimes:

• Ubuntu: 1min 30 sec
• Mac and Windows: > 4 min

I don’t think it’s worth keeping Mac and Windows going forward. But interesting to see it in action.

Build Passing on Windows
I can confirm via CI that build now runs and passes on Windows.

ESLint Issues on Windows
I’m fighting with the linter on Windows. Here’s where I’m at:

• in order to run the lint script on Windows, I had to change script "yarn eslint ‘./packages’” to "yarn eslint packages” Now works but 😬
• I’m now getting line ending errors for all files. See https://github.com/redwoodjs/redwood/pull/363/checks?check_run_id=554129316

I actually believe the line ending errors might have to do with git when files are checked out to the Windows system. I don’t have my Windows virtual machine setup and it’s too time consuming to keep testing via GH Actions. Could you try the following locally:

• does yarn lint pass on Windows locally? If yes, then it’s likely git on the GH Action Windows runner. We’ll probably need to create a .gitattributes file with *.js text eol=lf. (And/or maybe this will be a problem anytime the repo is checked out to Windows system?)
• for supporting Windows contributors, we’ll probably need .editorconfig like we setup here for App
• last thought for now, since Prettier v2 now defaults to lineEndings = lf we should be fine to not set it explicitly… right?

This article was helpful for ideas:
https://markoskon.com/line-endings-in-vscode-with-git-and-eslint/

To Dos (maybe)

• add .gitattributes
• add .editorconfig (and also include VS Code extension here Add VSCode extension recommendations to redwoodjs/redwood #367)
• Remove CI builds for Mac and Windows

[mohsen1]

I think gitattributes will solve this. I wish Windows wasn't this slow. How about running Windows only on master? Mac doesn't seem necessary to me.

[peterp]

@thedavidprice It seems that Windows is not working well with:
https://github.com/tleunen/eslint-import-resolver-babel-module, we use this package to resolve our babel aliases, and to group our imports into logical sections.

I had a look at the packages code to see if anything stood out, but nothing did. At the moment I'm at a loss to figure out why this isn't working.

[weaversam8]

@peterp @thedavidprice - just tested this on my local machine. In addition to the There should be no empty line within import group error you mentioned above, I'm also getting 3k lint errors from prettier/prettier telling me to remove carriage returns... not sure if this is an issue with a global Prettier config on my computer or what, but figured I'd pass it along.

Edit: it just occurred to me that this could be a result of my Git configuration, below.

file:C:/Program Files/Git/etc/gitconfig core.autocrlf=true

This git config option should convert LFs to CRLFs on clone locally. (I think this is either the default option or a very common option.) I wonder if it's possible to add an ESLint rule to ignore CRs on Windows only...

---

That said, I at least was able to yarn install and yarn build, so that's a step in the right direction! 🎉

[mohsen1]

I think the issue is within the order rule in eslint-plugin-import where assumption is made that end of line is "\n". Replacing that with require("os").EOL should fix it.

Made a PR to get this fixed over there. It might take a while to get it fixed. Maybe disable the rule for now? You can make eslint config disable it based on the environment.

[peterp]

I think the issue is within the order rule in eslint-plugin-import where assumption is made that end of line is "\n". Replacing that with require("os").EOL should fix it.

@mohsen1 I would have imagined that the .gitattributes line would've changed all the imported files to be LF instead of CRLF.

The error seems to indicate that there is a blank line, which separates the imports, and there shouldn't be, so the problem is that the imports aren't detected properly... My inner nerd really wants to understand this! (I need to confirm that my assumption about the error is correct thought.)

[peterp]

This git config option should convert LFs to CRLFs on clone locally. (I think this is either the default option or a very common option.) I wonder if it's possible to add an ESLint rule to ignore CRs on Windows only...

@weaversam8 I would have thought that a .gitattributes file would overwrite the global settings, and I think LF is the desired way to go about this... That said I'm not 100% sure about both of those assumptions.

Happy to hear that you're able to build! Now we just need to fix linting on windows :P

[thedavidprice]

@peterp I suggest we separate this effort into two parts, build and lint. That way I can move us forward:

1. adjust this PR to include only the working "build" changes, and then merge
2. create a new PR and port the "lint" work to address separately

That way we can at least provide Windows users the capability to build. And merge the PRs dependent on this one.

re: .gitattributes
Has anyone tested as follows:

1. re-cloned/forked this branch locally (since Peter committed .gitattributes file)
2. and then run lint tests locally or using GH actions?

I'm going to set up a Windows environment to do this myself. Just haven't had the time (yet).

[thedavidprice]

yarn build is working

Confirming this runs locally on both Mac and Windows.

yarn lint errors on Windows

Adding .gitattributes has eliminated the CRLF errors during the GH Action run, but has new error There should be no empty line within import group

Locally, after a fresh clone, Windows has an issue with files showing as modified and also still has CRLF errors in Powershell.

merging build fixes, opening PR for working on lint issues

I’ve checked out a new branch based on this one to continue the work on Windows system ‘yarn lint’.

See #395