Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade deps, add storybook #4947

Merged
merged 23 commits into from
Jan 13, 2025
Merged

Upgrade deps, add storybook #4947

merged 23 commits into from
Jan 13, 2025

Conversation

ukutaht
Copy link
Contributor

@ukutaht ukutaht commented Jan 7, 2025

Changes

Adds phoenix storybook dependency along with example pages for dropdowns and basic inputs. In order to make it work I had to upgrade some deps:

  • phoenix_ecto to 4.5
  • phoenix_html to 4.1
  • phoenix_live_view to 1.0

There were some breaking changes which I'll take notes on in self-review

There are a couple known issues with storybook installation:

  1. The storybook area (./storybook) uses its own JS bundle which is using pre-1.0 version of liveview. This causes a console warning but no real-world issues as far as I can tell. A new version of storybook should be released this week with support for Liveview 1.0
  2. For some reason, when you navigate to a page with Alpine.JS component (Input -> Dropdown) it causes some error from AlpineJS. When you load the page with AlpineJS directly it works fine. I wasn't able to find a fix.

@@ -30,9 +30,6 @@
"d3": "^7.9.0",
"dayjs": "^1.11.7",
"iframe-resizer": "^4.3.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should have been removed when we moved to esbuild. Modern phoenix config wants these JS packages to be resolved via mix ../deps folder but our package.json was actually shadowing the same deps in ./node_modules.

By removing from node_modules we have less config and guaranteed that the frontend package has the same version as the backend one.

lib/plausible/billing/ecto/feature.ex Outdated Show resolved Hide resolved
lib/plausible/billing/ecto/limit.ex Outdated Show resolved Hide resolved
@ukutaht ukutaht requested review from apata, aerosol, zoldar and a team January 7, 2025 14:06
@ukutaht ukutaht added the preview label Jan 7, 2025
Copy link

github-actions bot commented Jan 7, 2025

Preview environment👷🏼‍♀️🏗️
PR-4947

Copy link
Member

@aerosol aerosol left a comment

Choose a reason for hiding this comment

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

Cool work 👏.

CI failures aside, I think this could benefit from being a MIX_ENV=dev-only dependency/runtime. Or do you intentionally want to run it on prod servers? IMHO it's unnecessary build clutter for prod images.

I've tried looking into the Alpine misbehavior, but Alpine-specific setup you did seems a-OK.

@ukutaht
Copy link
Contributor Author

ukutaht commented Jan 8, 2025

CI failures aside

About that - the new HeeX engine wants us to convert all the templates from <%= var %> to {var} with mix format. However this will make the PR much harder to review. I was thinking I'd wait to get reviews and push the mix format change at the last minute before merging.

I think this could benefit from being a MIX_ENV=dev-only dependency/runtime. Or do you intentionally want to run it on prod servers? IMHO it's unnecessary build clutter for prod images.

Yeah it feels like a dev thing to me as well but there were a couple points that made me include it in docker builds:

Maybe just including in preview builds would be a good solution for now. But since preview builds are also made with MIX_ENV=prod it would only be a runtime exclude so not really address the 'build clutter' issue which I agree with. BTW I found that we have a couple instances in the codebase checking for MIX_ENV=staging which, as far as I can tell, is never the case.

Anyways - I definitely think having storybook in preview environments is useful but I'm agnostic about prod. Given all that - do you think we should exclude from prod at runtime?

@apata
Copy link
Contributor

apata commented Jan 8, 2025

I think being able to view the storybook without running the app locally is an important part for its adoption: when following the storybook is inconvenient, it's unlikely to help in unifying user experience within the app.

That said, I'd also be wary of exposing it from prod servers: as I see it, the increase in attack surface / ways for our prod servers to fail is not worth the convenient plausible.io/storybook URL. Have it on staging?

@ukutaht ukutaht force-pushed the dropdown-storybook branch from 27b0a6e to fb8dbe3 Compare January 9, 2025 11:24
@ukutaht
Copy link
Contributor Author

ukutaht commented Jan 10, 2025

I've pushed the large mix format commit.

PS: Merging this will likely cause conflicts with other PRs that change templates

@ukutaht ukutaht force-pushed the dropdown-storybook branch from 50501aa to 81f096d Compare January 10, 2025 10:07
Copy link
Contributor

@apata apata left a comment

Choose a reason for hiding this comment

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

All seems to work as expected in the review env.

@ukutaht ukutaht force-pushed the dropdown-storybook branch from 81f096d to 6f5df63 Compare January 13, 2025 12:08
@ukutaht ukutaht added this pull request to the merge queue Jan 13, 2025
Merged via the queue into master with commit 6c30f62 Jan 13, 2025
8 checks passed
@ukutaht ukutaht deleted the dropdown-storybook branch January 13, 2025 12:39
@ukutaht ukutaht mentioned this pull request Jan 13, 2025
ukutaht added a commit that referenced this pull request Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants