Azure Pipelines support (Linux, macOS, and Windows)

[willsmythe]

Azure Pipelines configuration for building and testing create-react-app on Linux, Windows, and macOS.

1. Runs all test suites on Linux, macOS, and Windows on Node 8 and 10 (see recent results).

2. Uses a custom version of Verdaccio (for now) which enables us to set HTTP Agent options (request/agentOptions). This is necessary to work around an Azure VM specific issue caused when too many sockets are opened. See https://docs.microsoft.com/en-us/azure/app-service/app-service-web-nodejs-best-practices-and-troubleshoot-guide#my-node-application-is-making-excessive-outbound-calls

3. Fixes additional problems not specific to Azure Pipelines that are currently causing builds on Travis to fail:

   • Error involving different versions of Jest (24.5.0 vs. 24.6.0): The react-scripts package provided by Create React App requires a dependency: "jest": "24.5.0" ... However, a different version of jest was detected higher up in the tree: /home/travis/build/facebook/create-react-app/node_modules/jest (version: 24.6.0). Caused by a new version of Jest (24.6.0) in the last 24 hours and a mix of 24.5.0 and ^24.5.0 in the various package.json
   • Cannot find module '@babel/plugin-transform-react-jsx after ejection (see Build fails after eject: Cannot find module '@babel/plugin-transform-react-jsx' facebook/create-react-app#6099)

[ianschmitz]

I added a dummy azure-pipelines.yml back to master as the DevOps connection was broken without it.

I notice that in this PR there is a . prefix for the config file. DevOps seems to pick up the one without the prefix. Should we rename the files in your PR?

[willsmythe]

@ianschmitz - I renamed the YAML files. There's a way to get Azure Pipelines to use an alternate name, but probably not worth the hassle :)

[willsmythe]

At quick glance, most of the Azure Pipelines build failures are caused by the same problem discussed in item 3 above (a new minor release of jest and babel-jest). The error shows up like this:

The react-scripts package provided by Create React App requires a dependency:

  "babel-jest": "24.6.0"

Don't try to install it manually: your package manager does it automatically.
However, a different version of babel-jest was detected higher up in the tree:

  /home/vsts/work/_temp/tmp.AQLJ720ftV/test-app/node_modules/babel-jest (version: 24.7.0) 

Although my PR "pinned" the various package.json files to "24.6.0", there must be something else producing/generating a "^24.6.0", which then causes a mix of 24.6.0 and 24.7.0 (and this error message).

[ianschmitz]

The react-scripts package provided by Create React App requires a dependency:

"babel-jest": "24.6.0"

Don't try to install it manually: your package manager does it automatically.
However, a different version of babel-jest was detected higher up in the tree:

/home/vsts/work/_temp/tmp.AQLJ720ftV/test-app/node_modules/babel-jest (version: 24.7.0)

I quickly looked into this issue as it's blocking other PRs, and i suspect it's due to this dependency here: https://github.com/facebook/jest/blob/f3dab7cd92006540ecee5a0b8b6811608861bd52/packages/jest-config/package.json#L16. Since we pin the version of jest, when a new version of jest comes out, it pulls in jest-config through other transient dependencies, which pulls in the latest version of babel-jest. I've experienced this a number of times the last couple days as a couple jest versions came out in rapid succession while i was working on facebook#6654. I would update our jest/babel-jest versions, and as soon as a new version of jest came out it triggered the preflight warning again in CI.

Here's an example of our dependencies in a fresh 3.0 alpha app:

test-app@0.1.0 /c/projects/test-app
└─┬ react-scripts@3.0.0-next.b0cbf2ca
  ├── babel-jest@24.5.0
  └─┬ jest@24.5.0
    └─┬ jest-cli@24.5.0
      └─┬ jest-config@24.5.0
        └── babel-jest@24.5.0  deduped

I think perhaps the best solution here may be to use a carat version reference to babel-jest in our react-scripts package.json. This should then be deduped out of react-scripts into the root of node_modules in the project, which will get rid of the preflight warning. @SimenB does this sound like a good solution for CRA?

[SimenB]

Yeah, I think so!

---

Not sure why you "lock" down your dependencies - unless you ship a lockfile as well it's not really doing anything as almost every single one of your dependencies are going to be using version ranges

[ianschmitz]

Not sure why you "lock" down your dependencies

Yeah it's something we're going to review as a team after our 3.0 release is out the door. Thanks for all the help over the last few weeks!

[willsmythe]

@ianschmitz - btw, I previously experimented with softening the logic in https://github.com/ianschmitz/create-react-app/blob/master/packages/react-scripts/scripts/utils/verifyPackageTree.js#L77 --- basically changing the version comparison from a "string equals" to a "is this compatible". I didn't take this too far since I don't have a full understanding of the ramifications of having even slightly different versions of jest, babel-jest, etc, but maybe there are optimizations that can be made in this logic to solve this problem.

[ianschmitz]

/cc @iansu @mrmckeb @Timer @amyrlam @petetnt @bugzpodder @heyimalex - sorry for the spam. Would be great to get your feedback on this.

We're aiming to get good coverage across all 3 platforms as we often get Windows specific issues reported for example.

The test suite reporting you may see at the bottom of the PR page gets rolled up into one big green checkmark if all pass, so don't get too worried about the large number of build status reports there.

Once we're happy with this we can merge it into my fork and I'll PR back into CRA proper and setup the Azure DevOps connection at that time.

[mrmckeb]

This looks great, @ianschmitz. A lot of effort has gone into this and I think it'll really pay off.

Minor detail, maybe we need to add EOL to an .editorconfig, I noticed tasks/verdaccio.yaml missing one.

[petetnt]

Looks solid to me, really nice work with this, it will most definitely pay off

[willsmythe]

Thanks for the feedback. I'll fix the EOL in verdaccio.yaml.

Regarding the number of jobs ad build time ...

The times for the latest build (of master in my fork) look good: start to finish time is 19 minutes. This includes the 5 big test suites across Node 8 and 10 on Linux, macOS, and Windows (30 jobs) and the "old Node" test suite on Linux, macOS, and Windows (3 jobs).

It's your call, but if you don't really need to test on every combination of suite/Node/OS, I wouldn't. One benefit in your case is reducing the risk of an occasional Verdaccio or network problem causing a job [and therefore the entire build] to fail.

Let me know if you want to see any other changes ...

[ianschmitz]

Some additional context to the Verdaccio issues on Windows build agents that @willsmythe provided me earlier:

think it’s related to this: https://docs.microsoft.com/en-us/azure/app-service/app-service-web-nodejs-best-practices-and-troubleshoot-guide#my-node-application-is-making-excessive-outbound-calls

This is a known issue, specific to Windows VMs on Azure.

I was able to get a create-react-app build working by patching Verdaccio with custom HTTP Agent options. Verdaccio uses “request”, so I am just hard-coding the options like this:

    const req = request(
      {
        url: uri,
        method: method
        headers: headers,
        body: json,
        ca: this.ca,
        proxy: this.proxy,
        encoding: null,
        gzip: true,
        timeout: this.timeout,
        strictSSL: this.strict_ssl,
+       agentOptions: {
+         keepAlive: true,
+         maxSockets: 40,
+         maxFreeSockets: 10
+       }

Once I have good confidence in this approach, we can discuss options. But, it’s looking like Verdaccio needs to open up the ability for options to be provided via configuration (which shouldn’t be a hard or controversial change).

@willsmythe assuming this fix makes it into Verdaccio proper, it sounds like you still anticipate the occasional network failures on the Windows agents. Is that correct? If so do you have any suggestions to improve this either on our side or within the Windows build agents?

[willsmythe]

@willsmythe assuming this fix makes it into Verdaccio proper, it sounds like you still anticipate the occasional network failures on the Windows agents. Is that correct? If so do you have any suggestions to improve this either on our side or within the Windows build agents?

I don't think I have seen any recent build failures related to network/connectivity, so I feel pretty good about the Verdaccio fixes. I have an uneasy feeling about Verdaccio in general (not just on Windows), but this mainly comes from seeing other reported issues with Verdaccio, especially under load.

Overall, I don't think there is a problem here. I do think our upcoming caching support will be useful in reducing the calls to Verdaccio and registry.npmjs.org (which is always a good thing to do).

[iansu]

This looks great. Good job everyone!

In terms of reducing the number of builds, we currently only run the behavior test suite on Mac. The old-node test suite really just needs to run on one platform (probably linux is the easiest) with Node 6. I think that eliminates ~8 builds.

[willsmythe]

I pushed a few changes to simplify the Azure Pipelines YAML. Mainly this will make it easier to add other common steps (like a step to publish test or coverage results) down the road.

This also [hopefully] fixes a random problem where yarn/npm would fail with an unexpected end of file (usually for react-script-forks .tgz). The root cause was a connectivity problem between the yarn/npm clients and the local Verdaccio HTTP server (so not an issue between Verdaccio and registry.npmjs.org). This issue is described here: verdaccio/verdaccio#301. The workaround was setting keepAliveTimeout to 0 in the Verdaccio config. I also updated the maxage property to 30m to avoid Verdaccio going back to registry.npmjs.org for a package it already pulled down. I picked this timeout since it should generally cover the duration of the build.

[willsmythe]

Sounds good to me. I just pushed this change.

[iansu]

@willsmythe Sorry, I don't think I was clear about the behavior test suite. We run that test suite on Mac and all other platforms, but it's the only test suite we run on the Mac. All the others are Linux and Windows only.

[willsmythe]

Here's the latest build on my fork (with the updated configurations/jobs): https://dev.azure.com/willsmythe/create-react-app/_build/results?buildId=996

[ianschmitz]

Hey @willsmythe. We just shipped 3.0 of CRA, so now we're able to focus on getting this in!

I just merged master in and it looks like all the builds are failing because of the git status check. I haven't had time to dig into it yet but will do tomorrow night if you haven't figured it out by that point.

EDIT: Just thinking... we updated Lerna just before we released 3.0. That may have changed things here since it looks like it's saying as part of the git status that all of the package.json files have been modified.

[willsmythe]

Congrats on 3.0 :) I added a line to echo the current git diff so we can see what's changing.

[willsmythe]

Based on the breaking changes described in the Lerna 3.0 change log, I played around with replacing --skip-git with --no-push. May also need to do something with --no-commit-hooks.

I won't have much time over the next few days to dig much more into this (fyi).

Related to this, I also noticed a few deprecated warnings related to Lerna 3.0 ...

WARN deprecated --cd-version has been replaced by positional [bump]
WARN deprecated --skip-git has been replaced by --no-git-tag-version --no-push
WARN deprecated --npm-tag has been renamed --dist-tag

[willsmythe]

@ianschmitz - I figured out the issue with Lerna 3.x and Azure Pipelines. Long story short, the following change in Lerna 3.13.2 lerna/lerna@a7ad9b6 caused a change in behavior related to publishing. It only showed up in Azure Pipelines because Azure Pipelines doesn't (by default) clone into a directory with the same name as the repo. This messed up Lerna's "has rooted" leaf logic (which is new/different in Lerna 3.13.2):

this.hasRootedLeaf = this.packageGraph.has(this.project.manifest.name);

this.project.manifest.name was s (the name of the directory Azure Pipelines cloned into). The fix was to override the default checkout/clone directory in the Azure Pipelines YAML:

  - checkout: self
    path: create-react-app

A better (more universal) fix would be to set this.project.manifest.name to "create-react-app", but I'm not sure what the impact of this would be (or where to do it).

This change should probably have been noted as a breaking changed in the Lerna changelog as well, fwiw.

[ianschmitz]

Woohoo! Thanks @willsmythe.

I'm going to merge this in to my fork now. There were a couple things i wanted to clean up in the publish script that i'll do in another PR and tag you in. Once that's in we'll do a PR back into CRA proper.

I'll keep in touch while we finalize this.