@uppy/companion: don't pin Yarn version in package.json

[aduh95]

The Dockerfile tries to build Companion standalone using some
trickery that is very much tied to npm logic, Yarn is not the fittest
tool here.

[mifi]

What's this trickery you're referring to? I just tried locally on my machine, and even though I have installed uppy with corepack yarn install, I was able to run npm run build successfully and even corepack yarn run build works for me. I think we should strive for equal environments in development and prod (if we use yarn in dev, we should use yarn in prod too)

[aduh95]

What's this trickery you're referring to?

uppy/packages/@uppy/companion/Dockerfile

Lines 9 to 17 in 8ebae54

	ADD package.json package-*.json yarn.* /tmp/
	RUN cd /tmp && apk --update add --virtual native-dep \
	make gcc g++ python3 libgcc libstdc++ git && \
	corepack yarn install && \
	apk del native-dep
	RUN mkdir -p /app && cd /app && ln -nfs /tmp/node_modules
	RUN apk add bash
	COPY . /app
	ENV PATH "${PATH}:/app/node_modules/.bin"

[mifi]

Is there any error? It’s not obvious to me why that code doesnt work with corepack yarn install but works with npm install

[aduh95]

I think we should strive for equal environments in development and prod

OK but where's the limit? Should all Uppy devs be forced to use Docker so we can have the same environment as the prod? The Dockerfile is using apk, which I don't have in my env, and I honestly don't see this as a problem.

if we use yarn in dev, we should use yarn in prod too

Why not use both? I think all Uppy devs have both npm and yarn on their path, so it's not an issue of a different environment, it's simply different tools.
Anyway, I'm not against using Yarn here, I tried and failed so I'm no longer interested in making this happen. I believe rolling back to npm makes sense here, and has the least risk associated with it.

why that code doesnt work with corepack yarn install but works with npm install

Yarn by default uses PnP instead of node_modules, we have to specify in its config file to use the node_modules linker, and we don't copy that config file into the docker container. Also, corepack yarn build will refuse to run if it can't find a yarn.lock file, while npm run build tries to run the script without any check. I don't know why it was decided to use a /tmp directory here (I asked @kiloreux but they didn't know/remember), and I'm too afraid to touch it at this point.

If you want this script to use Yarn, you'd need to refactor the Dockerfile so it pulls up the whole monorepo and install all the deps from all the packages (unless you can find a way to ask Yarn to install only a subset of those, not sure).

[mifi]

I agree that using yarn npm run build vs npm run build doesn't make any difference, I was thinking more about the differences between yarn install vs npm install.

OK but where's the limit? Should all Uppy devs be forced to use Docker so we can have the same environment as the prod? The Dockerfile is using apk, which I don't have in my env, and I honestly don't see this as a problem.

Absolutely, i agree that we shouldn't have to run companion in docker or with apk in our dev environment. but I thought that things like using a supported node.js version and the same package manager as production for installing node_modules is more important.

Why not use both? I think all Uppy devs have both npm and yarn on their path, so it's not an issue of a different environment, it's simply different tools.

Drawbacks I see with using two different package managers for node_modules:

• lockfile is not respected, so docker will have a different set of node_modules dependencies than dev
  • although maybe we can work around by converting yarn.lock to package-lock.json in the docker build script? need to research how to do that
• I thought yarn and npm had some differences in how they resolve dependencies when it comes to directory structure and deduplication, and this may affect some packages that are sensitive to the node_modules structure. maybe this has changed in recent versions though...

Yarn by default uses PnP instead of node_modules, we have to specify in its config file to use the node_modules linker, and we don't copy that config file into the docker container.

I agree we should strive to use PnP as long as it's possible. I know some packages don't support PnP but we haven't hit that issue yet, so it makes more sense to rewrite the dockerfile to support PnP than to turn off PnP.

If you want this script to use Yarn, you'd need to refactor the Dockerfile so it pulls up the whole monorepo and install all the deps from all the packages (unless you can find a way to ask Yarn to install only a subset of those, not sure).

I think we can use yarn workspaces focus to install only one workspace's dependencies. I've used it in other projects with success. (non-PnP project but I believe it works for PnP too.)

I can see why nobody wants to do the job of rewriting the dockerfile, so maybe we should just merge this and raise a new TODO issue for rewriting it to support yarn. what do you think?

[aduh95]

• lockfile is not respected, so docker will have a different set of node_modules dependencies than dev

AFAIU it was already the case, the Dockerfile was not getting the lock file when we were using npm. That doesn't mean we should keep it this way, but at least it's not new with this very PR.

I agree we should strive to use PnP as long as it's possible. I know some packages don't support PnP but we haven't hit that issue yet, so it makes more sense to rewrite the dockerfile to support PnP than to turn off PnP.

We are currently not using PnP, it's already disabled. If we were, npm run … commands would not work. We could decide to switch to it, but that's a discussion for another time.

I can see why nobody wants to do the job of rewriting the dockerfile, so maybe we should just merge this and raise a new TODO issue for rewriting it to support yarn. what do you think?

That's fine by me; maybe I could add a TODO comment to save ourselves from an issue, wdyt?

[aduh95]

Docker deploys are being fixed in #3355, this PR now focuses only on fixing #3356.