Skip to content

Conversation

@webbertakken
Copy link

@webbertakken webbertakken commented Oct 28, 2022

Add caching to the default workflow.

This is a pretty standard step I'd say that many people (and machines) would benefit from.

Before:
image

After (from second run after implementing it)
image

See it in action: webbertakken/map-editor-2d#17

Changes are reflected here: https://deploy-preview-954--tauri.netlify.app/v1/guides/building/cross-platform#example-workflow

@webbertakken webbertakken changed the title Update cross-platform.md docs: add caching to cross-platform example workflow Oct 28, 2022
@github-actions
Copy link

github-actions bot commented Oct 28, 2022

🚀 Deployed on https://dev954--tauri.netlify.app

@github-actions github-actions bot temporarily deployed to pull request October 28, 2022 17:47 Inactive
@FabianLars
Copy link
Member

This was discussed multiple times already, and i feel like it was always me who blocked it 😂
The problem i have with this is that it only offers value if it runs at least once a week, which i think should be rather rare. If it's less than once a week this will actually increase CI times.
So while i think this is nice for workflows that run tests, or for nightly releases, i don't think there is much value for the actual release workflow.

So imo this would be better suited for the testing example in the tauri-action repo or a new testing-in-CI guide (the latter probably doesn't offer much value idk)

What do you think?

@webbertakken
Copy link
Author

webbertakken commented Oct 28, 2022

Personally I prefer to help newer developers in a way that they can just delete the steps they don't want, instead of puzzle (during a learning curve) to get the best setup.

In my opinion complete examples are useful. The official docs for a tool usually show close to "the best" approach, and is fine-tuned by the community if any mistakes are made. For that reason alone I think it's worth adding it.

Besides that I think it's globally encouraged to iterate quickly and "release often". Many people might release a few times one week, and not at all for a few weeks after that. (often the case for my projects, 1 minor release and quickly after one or two patch releases when people notice bugs).

I mean adding it would mean:

Upsides:

  • The rest of the week your releases are 80% faster
  • Which makes it easier to test changes to your workflow as well
  • Much easier to understand a complete production grade workflow for beginners

Downsides:

  • Slightly harder to understand the example at first sight
  • 10 seconds (?) slower on cache miss
  • Maintenance of the added docs

That said. It's completely up to you. I'm just trying to be helpful towards newer devs who struggle with this kind of thing 😆

Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

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

Sooo, i still don't agree (basing this on what i've seen in the community btw), but honestly i'm getting a bit tired of people asking for a cache (and then not using it in the end), so let's get it in anyway :D (interpret this as listening to the community :P )

I added 2 suggestions i'd like to hear your thoughts on.
Last thing that's not really related to this PR but i think we should add a comment or something to the yarn && yarn build step (or change the default), because the default template has yarn build configured as the beforeBuildCommand too so we're effectively building the frontend twice which depending on the frontend can add quite some time.

Co-authored-by: Fabian-Lars <[email protected]>
@github-actions github-actions bot temporarily deployed to pull request October 29, 2022 21:40 Inactive
@webbertakken
Copy link
Author

Haha, perhaps it'll be good, or perhaps you'll have people asking to remove it next.
I suppose merging it is a great way to find out. 🙂

Thanks for your consideration.

@webbertakken
Copy link
Author

@FabianLars are you merging this?

@FabianLars
Copy link
Member

That's the plan yes. I did some more testing a few days (or weeks?) ago, trying to make it easier to configure and not yarn specific, but weren't able to finish it up yet (had to focus on other stuff etc).

At this point i am not sure if we should even include a default (frontend) caching solution in the snippet but instead of a section explain possible solutions, reasons;

First of all, we have to have the target audience in mind, most never worked with CI before, some of them not even with frontend stuff - this is btw the reason why i proposed the setup-node cache. Most people just use this action because there is no cross-compilation!

As i said above, i tried to make it easier or more general, for example by using npm, yarn and pnpm lockfiles for the cache key and potentially using setup-node for the global cache (so that we don't have to get&cache the path ourselves).
This worked alright except for pnpm which fails on Windows with this approach (invalid symlinks or something i guess).

Speaking of pnpm, not caching node_modules (but the global cache) is usually faster, or just as fast, because it installs the dependencies faster than the cache action can download, decompress and copy the node_modules folder.

Then we also have the issue of npm ci (which people should probably use?) where caching node_modules/ is harmful because it gets deleted before installation automatically.

Please let me know if you have any ideas. If not i'd tend to go for the setup-node cache for now and figure out a good approach later after cleaning up the docs some more.

@webbertakken
Copy link
Author

I see. Good effort!

As for my input, I love creating a quick PR to open source projects and try and contribute a small iteration, but I do not have the capacity to stay deeply in context for each PR. Mostly I was just asking to see whether I should keep my fork around or not.

I have no strong preference towards one either solution. Please do feel free to apply your suggestion.

@FabianLars
Copy link
Member

Okay let's make it quick and easy then, i'll use setup-node and re-iterate as soon as i can.

Thank you for your contribution and sorry for the super long delay!

@FabianLars FabianLars merged commit e6fab3a into tauri-apps:dev Dec 1, 2022
@github-actions github-actions bot temporarily deployed to pull request December 1, 2022 16:21 Inactive
@webbertakken
Copy link
Author

You're welcome! And thank you so much for your effort as well as maintaining this package in general.

@webbertakken webbertakken deleted the patch-1 branch December 1, 2022 18:21
lorenzolewis added a commit that referenced this pull request Dec 6, 2022
* fix(deps): update dependency docs-searchbar.js to v2.4.1 (#996)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* More idiomatic NixOS setup (#995)

* directly reference clap link for the features doc card list (#997)

* fix(deps): update dependency react-markdown to v8.0.4 (#998)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* docs: add caching to cross-platform example workflow (#954)

* Update cross-platform.md

* remove shared-key

Co-authored-by: Fabian-Lars <[email protected]>

* Update docs/guides/building/cross-platform.md

Co-authored-by: Fabian-Lars <[email protected]>

* Add optimization tip for "compression" feature (#930)

* Move to using combined locale and two-digit codes for i18n

* Add Crowdin admin note for i18n

* I blame @FabianLars if it breaks anything (#999)

* fix: Update github action guide (#1000)

* fix: Update github action guide

* Update cross-platform.md

* ci: Update actions in update-docs workflow (#1002)

* Add args example to sidecar doc (#1003)

Add a section in sidecar doc to demo how to pass args to sidecar cmds

* update git submodule

* fix(generator): adapt renamed property

* ci: Always pull latest version of submodule

* chore(docs): Update Rust & TS docs (#1004)

Co-authored-by: FabianLars <[email protected]>

* Updated Crowdin config to include more Guides pages (#1005)

* Updated Crowdin config to include more Guides pages

* Fixed the path to updater.md

* Feat/i18n banner (#1006)

* Add i18n banner

* Fix styles

* Fix i18n preview

* Change i18n preview to use locale instead of twoLetterCode

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Ben Buscarino <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
Co-authored-by: Webber Takken <[email protected]>
Co-authored-by: Fabian-Lars <[email protected]>
Co-authored-by: Will <[email protected]>
Co-authored-by: Kenneth <[email protected]>
Co-authored-by: tauri <[email protected]>
Co-authored-by: FabianLars <[email protected]>
Co-authored-by: DK Liao <[email protected]>
FabianLars added a commit that referenced this pull request Dec 9, 2022
* fix(deps): update dependency docs-searchbar.js to v2.4.1 (#996)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* More idiomatic NixOS setup (#995)

* directly reference clap link for the features doc card list (#997)

* fix(deps): update dependency react-markdown to v8.0.4 (#998)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* docs: add caching to cross-platform example workflow (#954)

* Update cross-platform.md

* remove shared-key

Co-authored-by: Fabian-Lars <[email protected]>

* Update docs/guides/building/cross-platform.md

Co-authored-by: Fabian-Lars <[email protected]>

* Add optimization tip for "compression" feature (#930)

* Move to using combined locale and two-digit codes for i18n

* Add Crowdin admin note for i18n

* I blame @FabianLars if it breaks anything (#999)

* fix: Update github action guide (#1000)

* fix: Update github action guide

* Update cross-platform.md

* ci: Update actions in update-docs workflow (#1002)

* Add args example to sidecar doc (#1003)

Add a section in sidecar doc to demo how to pass args to sidecar cmds

* update git submodule

* fix(generator): adapt renamed property

* ci: Always pull latest version of submodule

* chore(docs): Update Rust & TS docs (#1004)

Co-authored-by: FabianLars <[email protected]>

* Updated Crowdin config to include more Guides pages (#1005)

* Updated Crowdin config to include more Guides pages

* Fixed the path to updater.md

* Feat/i18n banner (#1006)

* Add i18n banner

* Fix styles

* Fix i18n preview

* Change i18n preview to use locale instead of twoLetterCode

* chore(deps): update dependency typescript to v4.9.4 (#1008)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(docs): Update Rust & TS docs (#1009)

Co-authored-by: tauri-bot <[email protected]>

* docs: fix typo (#1011)

* feat(blog): add 2.0.0-alpha.0 (mobile) announcement (#1012)

Co-authored-by: Lorenzo Lewis <[email protected]>

* fix: add rust target command to macos build guide (#1015)

* fix: add rust target command to macos build guide

* typo

* fix prereq sidebar layout and link to it from mobile docs

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Ben Buscarino <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
Co-authored-by: Webber Takken <[email protected]>
Co-authored-by: Will <[email protected]>
Co-authored-by: Lorenzo Lewis <[email protected]>
Co-authored-by: Kenneth <[email protected]>
Co-authored-by: tauri <[email protected]>
Co-authored-by: FabianLars <[email protected]>
Co-authored-by: DK Liao <[email protected]>
Co-authored-by: tauri-bot <[email protected]>
Co-authored-by: Ayanava Karmakar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants