feat: use pnpm #1789

Merged
konrad merged 7 commits from dpschen/frontend:feature/feat-use-pnpm into main 2022-09-21 20:49:14 +00:00
Member

Not sure about deployment.

Not sure about deployment.
Owner

I've never used pnpm but it looks quite promising!

If we switch to it, we should switch over completely - that also includes the dockerfile and the CI. We also need to update the developer docs.

I've never used pnpm but it looks quite promising! If we switch to it, we should switch over completely - that also includes the dockerfile and the CI. We also need to update the [developer docs](https://vikunja.io/docs/development/#frontend-requirements).
Owner

I think the build image we use in CI also needs an upgrade.

I think the build image we use in CI also needs an upgrade.
dpschen force-pushed feature/feat-use-pnpm from b008e106d1 to 0bcfbdbacb 2022-04-13 19:29:27 +00:00 Compare
dpschen force-pushed feature/feat-use-pnpm from 0bcfbdbacb to 922badd4da 2022-04-13 19:40:50 +00:00 Compare
Owner

Seems fine to me. The only thing missing now is the pipeline image and likely the docker image itself too.

We'll either need to use an image from someone else or build one ourselves. @dpschen Do you know if there are any official docker images from pnpm?

Seems fine to me. The only thing missing now is the pipeline image and likely the docker image itself too. We'll either need to use an image from someone else or build one ourselves. @dpschen Do you know if there are any official docker images from pnpm?
Author
Member
I did find this: https://pnpm.io/cli/fetch#usage-scenario
Owner

That looks like we'll need to add a RUN curl -f https://get.pnpm.io/v6.16.js | node - add --global pnpm to our Dockerfile and also build a simple docker image with that so we can use pnpm in the pipeline without having to install it on every pipeline step.

That looks like we'll need to add a `RUN curl -f https://get.pnpm.io/v6.16.js | node - add --global pnpm` to our `Dockerfile` and also build a simple docker image with that so we can use pnpm in the pipeline without having to install it on every pipeline step.
dpschen added the
help wanted
label 2022-05-21 14:54:56 +00:00
dpschen force-pushed feature/feat-use-pnpm from 922badd4da to 361e0e22d5 2022-07-02 16:07:03 +00:00 Compare
Owner

We'll need to upgrade renovate as well because only the newer versions support pnpm.

We'll need to upgrade renovate as well because only the newer versions support pnpm.
Author
Member

We'll need to upgrade renovate as well because only the newer versions support pnpm.

Where can this be upgraded?

> We'll need to upgrade renovate as well because only the newer versions support pnpm. Where can this be upgraded?
Owner

I'll need to do that on the server.

I'll need to do that on the server.
Owner

Just upgraded, renovate should now be able to handle pnpm dependencies.

Just upgraded, renovate should now be able to handle pnpm dependencies.
dpschen force-pushed feature/feat-use-pnpm from 361e0e22d5 to 508604abfd 2022-08-19 18:06:31 +00:00 Compare
dpschen force-pushed feature/feat-use-pnpm from 508604abfd to 4e7ff78d78 2022-08-19 18:28:00 +00:00 Compare
dpschen force-pushed feature/feat-use-pnpm from 4e7ff78d78 to be0b8c585f 2022-08-19 18:43:25 +00:00 Compare
dpschen force-pushed feature/feat-use-pnpm from be0b8c585f to fa61a33659 2022-08-19 18:50:01 +00:00 Compare
Member

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1789-feature-feat-use-pnpm--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1789-feature-feat-use-pnpm--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen force-pushed feature/feat-use-pnpm from fa61a33659 to f01d2bbb35 2022-08-19 20:01:04 +00:00 Compare
dpschen force-pushed feature/feat-use-pnpm from b46cb624e0 to 10a9f4a8cc 2022-08-20 09:42:59 +00:00 Compare
dpschen force-pushed feature/feat-use-pnpm from 10a9f4a8cc to 3748082fec 2022-08-20 09:48:19 +00:00 Compare
dpschen force-pushed feature/feat-use-pnpm from 3748082fec to 9d9e149f4a 2022-08-20 10:49:26 +00:00 Compare
dpschen force-pushed feature/feat-use-pnpm from 9d9e149f4a to ede85ad0dd 2022-08-20 11:24:36 +00:00 Compare
Author
Member

I finished this as far as I can.
For the caching issues I need some help.

I finished this as far as I can. For the caching issues I need some help.
dpschen requested review from konrad 2022-09-20 16:21:27 +00:00
konrad was assigned by dpschen 2022-09-20 16:21:30 +00:00
konrad reviewed 2022-09-21 14:20:37 +00:00
Dockerfile Outdated
@ -19,0 +15,4 @@
ENV PNPM_CACHE_FOLDER .cache/pnpm/
# pnpm fetch does require only lockfile

But we'll still need to copy all frontend files anyway?

But we'll still need to copy all frontend files anyway?
Author
Member

Yes. This just makes that step cachable.

Copying everything over happens in line 28.

Yes. This just makes that step cachable. Copying everything over happens in line 28.
konrad marked this conversation as resolved
Owner

About the caching: Can you update the PR once renovate pinned the just-now merged pina dependency? Then we should have a rather stable state to experiment until the pipeline works.

About the caching: Can you update the PR once renovate pinned the just-now merged pina dependency? Then we should have a rather stable state to experiment until the pipeline works.
Author
Member

Btw: very useful here: https://github.com/antfu/ni

Btw: very useful here: https://github.com/antfu/ni
dpschen force-pushed feature/feat-use-pnpm from ede85ad0dd to e5d04c98da 2022-09-21 15:29:49 +00:00 Compare
Author
Member

I updated the PR with pinned pinia

I updated the PR with pinned pinia
konrad reviewed 2022-09-21 16:23:33 +00:00
@ -45,2 +46,2 @@
YARN_CACHE_FOLDER: .cache/yarn/
CYPRESS_CACHE_FOLDER: .cache/cypress/
PNPM_CACHE_FOLDER: .cache/pnpm
CYPRESS_CACHE_FOLDER: .cache/cypress

Does it work if you re-add the / at the end?

Does it work if you re-add the `/` at the end?
Author
Member

Probably not:
https://docs.cypress.io/guides/references/advanced-installation#Binary-cache

If I remember correctly removing the / was already a test. I guess both works.
Since the version without / is mentioned in the docs I kept that.

Probably not: https://docs.cypress.io/guides/references/advanced-installation#Binary-cache If I remember correctly removing the `/` was already a test. I guess both works. Since the version without `/` is mentioned in the docs I kept that.
konrad added 1 commit 2022-09-21 18:29:38 +00:00
continuous-integration/drone/pr Build is failing Details
b542221dac
chore: don't cache node_modules
konrad added 1 commit 2022-09-21 18:34:25 +00:00
continuous-integration/drone/pr Build is failing Details
6624db1d49
chore: don't use node alpine image
Owner

My first guess was maybe caching node_modules in ci was the issue - cypress states in their docs this should not be done. Didn't work though.

My next guess is maybe different chrome library locations between alpine and the cypress image so I'm reverting that change now.

My first guess was maybe caching `node_modules` in ci was the issue - cypress [states in their docs](https://docs.cypress.io/guides/continuous-integration/introduction#Caching) this should not be done. Didn't work though. My next guess is maybe different chrome library locations between alpine and the cypress image so I'm reverting that change now.
Owner

My next guess is maybe different chrome library locations between alpine and the cypress image so I'm reverting that change now.

Aaand that didn't work either.

I'm very tempted to just add a cypress install to the pipeline....

> My next guess is maybe different chrome library locations between alpine and the cypress image so I'm reverting that change now. Aaand that didn't work either. I'm very tempted to just add a `cypress install` to the pipeline....
konrad added 1 commit 2022-09-21 18:42:56 +00:00
continuous-integration/drone/pr Build is failing Details
62e227c767
fix: explicitly install cypress
konrad added 1 commit 2022-09-21 18:52:37 +00:00
continuous-integration/drone/pr Build is passing Details
dfb3561310
chore: use node alpine image
This reverts commit 6624db1d49.
Owner

I'm very tempted to just add a cypress install to the pipeline....

That seems to work - I'd say let's just keep it that way.

> I'm very tempted to just add a `cypress install` to the pipeline.... That seems to work - I'd say let's just keep it that way.
Owner
@dpschen would you mind sending another PR to change the docs at https://kolaente.dev/vikunja/api/src/branch/main/docs/content/doc/development/development.md#frontend-requirements ?
konrad added 1 commit 2022-09-21 18:56:19 +00:00
continuous-integration/drone/pr Build is passing Details
ef6454483a
chore(ci): sign drone config
dpschen changed title from WIP: feat: use pnpm to feat: use pnpm 2022-09-21 19:42:09 +00:00
dpschen reviewed 2022-09-21 19:44:20 +00:00
@ -18,1 +14,3 @@
fi && \
fi
ENV PNPM_CACHE_FOLDER .cache/pnpm/
Author
Member

This is the only place where there is now a final /

This is the only place where there is now a final `/`
konrad merged commit f7ca064127 into main 2022-09-21 20:49:14 +00:00
konrad referenced this issue from a commit 2022-09-21 20:49:14 +00:00
konrad deleted branch feature/feat-use-pnpm 2022-09-21 20:49:14 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.