# Simplify package scripts
This PR's purpose is to raise a discussion surrounding our current
package scripts.
It includes some suggestions that aim to simplify the scripts and
hopefully bring a much more straightforward approach to developing and
contributing to Unleash.
Building (prod) should only happen **explicitly** and when needed.
## Before PR (current behavior)
- Clone the project;
- Open 2 terminals: One for `unleash` and another for
`unleash/frontend`;
- On `unleash`:
- Run `yarn` (which will also build, for some reason?);
- Run `yarn start:dev` to start backend in dev mode (`tsc-watch`);
- On `unleash/frontend`:
- Run `yarn` (which will also build, for some reason?);
- Run `yarn start` to start frontend in dev mode (`vite`);
So it seems to me like we build unnecessarily every time we install
dependencies. Neither dev scripts need to build the project, as backend
uses `tsc-watch` and frontend uses `vite`. I'm unsure why this is the
case, as building can take a very long time.
There's also some complexity in the way we need to split the terminal to
`cd` into `frontend` and treat it as a different project. The fact that
we have different script names is also confusing (`yarn start`, `yarn
start:dev`, etc).
## After PR
- Clone the project;
- Run `yarn` to install all dependencies;
- Run `yarn dev` to get started developing Unleash;
Running `yarn` should take care of everything needed to start
developing. This includes installing dependencies for frontend as well.
It should not build projects if we are not being explicit about it,
especially since we don't need to build them at this stage.
Running `yarn dev` should start the project in dev mode. This means
running both projects in `dev` mode, which for `backend` means running
`tsc-watch` and for `frontend` means running `vite`.
Here this PR attempts to provide a better DX by using
[concurrently](https://www.npmjs.com/package/concurrently) and
[wait-on](https://www.npmjs.com/package/wait-on) - This means both tasks
are ran simultaneously, stdout is labeled accordingly, and are stopped
together. It also means that `frontend` waits for `backend` to be
serving at `4242` before starting, since `frontend` starts pretty much
immediately with `vite` and `backend` takes a bit longer. Of course,
when the `backend` is hot-reloading you may still find some
`ECONNREFUSED`s on `frontend` stdout while it recompiles.
No more splitting your terminal and treating `frontend` as a separate
project.
## Discussion points
Maybe there's a better alternative to `tsc-watch`? I briefly explored
some alternatives and while they had a much faster starting speed,
hot-reload was sometimes slower. IMO we should aspire to run
`src/server-dev.ts` directly and only compile when needed.
Running `dev:backend` still serves a version of the frontend (at 4242).
**Why? Can we remove that behavior?**
I can't imagine a scenario in dev where we wouldn't want to run the
latest version of the frontend with `vite`.
~~**Note:** This PR removes all other out-of-scope scripts to focus on
this revamp. If we decide to merge it, we should evaluate what other
existing scripts we still want to include. May be a good opportunity to
clean up unused ones and only include the ones we really use. This
includes scripts that our GH actions rely on.~~
**Update:** In an effort to minimize impact surface of this PR and make
it a bit more ready for merging:
- It updates some docs in
2a4ff805e8
and
1bbc488251
to reflect our new simplified flow;
- It includes the old package scripts for now in
039bc04699;
- It updates some of our GH actions to reflect the new scripts in
7782cb9b12;
Given its current status I'll promote the PR to "ready for review".
I still think we should have a second look at our existing scripts and
GH actions to see what we really need and/or should adapt, but it should
be a team effort so we have a broader context. Maybe on a follow-up PR.
Does this require any changes to related projects (e.g. Enterprise)?
---------
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
This PR revamps e2e tests, while adding a new one for the interactive
demo guide:
- Bumps Cypress from `9.7.0` to `12.11.0`;
- Bumps Cypress GH action from `v2` to `v5`;
- Makes any adjustments needed;
- Fixes a lot of issues identified with existing tests;
- Adds new `demo.spec.ts` e2e test that covers the entire demo guide
flow;
**Note:** Currently does not include `demo.spec.ts` in the GH action, as
it
[fails](https://github.com/Unleash/unleash/actions/runs/4896839575/jobs/8744137231?pr=3656)
on step 2.13 (last step of "user-specific" topic). It runs perfectly
fine locally, though.
Might be placebo, but in general tests seem less flaky now and they may
even be faster (especially when not adding the `demo` one, which would
always take a long time).
I've copied most of the descriptions from what we did for batch metrics
under the Edge tag, however there are a couple of top level descriptions
that are "fine" but I'm definitely open to suggestions.
---------
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
## About the changes
https://github.blog/changelog/2022-10-24-npm-v9-0-0-released/ introduced
a breaking change in the way they handle files inside package.json which
caused some issues with the way we pack and distribute Unleash:
> npm pack now follows a strict order of operations when applying ignore
rules. If a files array is present in the package.json, then rules in
.gitignore and .npmignore files from the root will be ignored.
What we discovered is that when having a nested .gitignore (the one we
have inside frontend), `npm publish` was taking that nested .gitignore
into account (despite the fact that we also have a package.json with
files inside the same folder). We tricked this by removing the `build`
folder from `frontend/.gitignore` and instead adding it into the root
`.gitignore` which is being ignored by `npm publish` following what's
stated in the release note above.
-----------------
Co-authored-by: Gard Rimestad <gard@getunleash.io>
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
After discussions around e2e tests:
- Became clear that the intention is to test core functionality only
Removing 2 test files:
- notifications.spec.ts : because all PRs are run against the same
heroku backend - no guarantee that more notifications will not happen as
other tests run - so the test is flaky
- settings.spec.ts: depends on instance being enterprise and a flag on
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Adds proper typescript support.
Created reusable commands
Added README for cypress test
Refactored tests
Fixed bugs as I found them.
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
---------
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
## About the changes
This is based on @nunogois suggestion to split the build in two, so we
don't build the frontend every time we run `yarn`
e2e frontend tests were forced to run by modifying frontend/README.md
### Important files
Some github actions had to be updated to also build the frontend. The
Dockerfile building our docker image was also looked into but it should
work as is
## Discussion points
This is a potentially risky operation as we might overlook something
that requires building the frontend which might lead to invalid builds.
We need to make sure when we do this we don't have any release planned.
## About the changes
This enables strictNullChecks which will give us nice hints in our IDEs
to avoid introducing more and hopefully will encourage us to fix some of
the existing problems.
Also:
1. The compiler explicitly ignores these errors
2. The "null checks action" still verifies we're not introducing new
ones.
The combination of these two things should help us to reduce the number
of nulls
For testing that the action still works (cause it was modified), [a
commit](5c4b818d1a)
was added introducing a bunch of null check errors:
https://github.com/Unleash/unleash/actions/runs/4364648191/jobs/7632224720
## About the changes
This is a small improvement adding more context to the comment message
when this action fails.
---------
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
## About the changes
- Actions are not counting successfully:
https://github.com/Unleash/unleash/actions/runs/4181713524/jobs/7243938807#step:5:17
- This should fix an issue with running yarn concurrently (which might
be leading to the zero counts)
- Move the code to a GH action so it's easier to debug
- Added a diff of the reported errors to help the PR author identify the
potential cause
[](https://renovatebot.com)
This PR contains the following updates:
| Package | Type | Update | Change |
|---|---|---|---|
| [docker/login-action](https://togithub.com/docker/login-action) |
action | major | `v1` -> `v2` |
---
### Release Notes
<details>
<summary>docker/login-action</summary>
### [`v2`](https://togithub.com/docker/login-action/compare/v1...v2)
[Compare
Source](https://togithub.com/docker/login-action/compare/v1...v2)
</details>
---
### Configuration
📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).
🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.
♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.
🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.
---
- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box
---
This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/Unleash/unleash).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMDUuNCIsInVwZGF0ZWRJblZlciI6IjM0LjEwOC40In0=-->
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
