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

Use elm-watch #160

Closed
wants to merge 13 commits into from
Closed

Use elm-watch #160

wants to merge 13 commits into from

Conversation

magopian
Copy link
Contributor

@magopian magopian commented Sep 15, 2022

Replace parcel with elm-watch

TODO:

  • update Scalingo env vars: eg MATOMO_TOKEN -> VITE_MATOMO_TOKEN

@magopian magopian marked this pull request as draft September 15, 2022 10:20
@magopian
Copy link
Contributor Author

So, this is now working correctly, BUT:

  • no easy live-reloading on sass file change (we could hack something together using something like this but it's hairy)
  • no easy way to use env vars (we need to use esbuild's define i guess)

In the current context, I'd say that elm-watch works well, but esbuild is less than ideal (at least for now, for us).

@magopian
Copy link
Contributor Author

Ok, this should now be up for review @n1k0 , would you like to take it for a spin?
It seems to me that everything works as expected, bar one "detail": the scalingo build times out for some reason... The server SHOULD be running on the proper port and host, from my local tests, but still it fails. Do you have an idea what could cause that?

@magopian magopian marked this pull request as ready for review September 16, 2022 14:02
@magopian magopian requested a review from n1k0 September 16, 2022 14:02
Copy link

@lydell lydell left a comment

Choose a reason for hiding this comment

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

Really cool to see elm-watch used together with Vite!

lib/index.js Outdated Show resolved Hide resolved
postprocess.mjs Outdated Show resolved Hide resolved
build/main.js Outdated Show resolved Hide resolved
@magopian magopian changed the title [WIP] use elm-watch Use elm-watch Sep 20, 2022
Copy link
Member

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

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

Several remarks at this point:

  • Port used for dev server is 5173; this should be updated in README (or couldn't we juyst set it up to use 1234 as before?)
  • elm-watch seems young and promising, but also prone to changes in a near future; wouldn't we want it to settle a little before leveraging it in production?
  • The API page is broken
    image
  • matter of personal taste, but I really appreciated the all-inclusive, configuration-less approach of Parcel; in this patch, I find the configuration files are a little hard to understand

@@ -7,12 +7,12 @@
/public/icomoon/*.txt
/public/icomoon/*.zip
/public/icomoon/demo-files
/public/icomoon/demo-files
Copy link
Member

Choose a reason for hiding this comment

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

Why removing this line? We don't want to version icomoon HTML demo files…

"start:parcel": "parcel serve index.html --no-cache",
"start:dev": "npm run build:init && concurrently -n \"watch,parcel,server\" -c \"green,cyan\" \"npm run start:parcel\" \"npm run server:dev\"",
"start": "PARCEL_ELM_NO_DEBUG=1 npm run start:dev",
"start": "run-pty run-pty.json",
Copy link
Member

Choose a reason for hiding this comment

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

Note that this triggers this warning in iTerm; maybe we should document it in README

image

Copy link
Member

@n1k0 n1k0 Sep 28, 2022

Choose a reason for hiding this comment

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

Also, running this command doesn't tell me at what URL the app is served; we should also document this in README (yes navigating helps, but it's easier if the user already knows)

Comment on lines +73 to +74
- `VITE_SENTRY_DSN`: le DSN [Sentry](https://sentry.io) à utiliser pour les rapports d'erreur.
- `VITE_MATOMO_TOKEN`: le token [Matomo](https://stats.data.gouv.fr/) permettant le suivi d'audience de l'API.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It's annoying these must be prefixed; no way to avoid it?

@lydell
Copy link

lydell commented Sep 28, 2022

young and promising, but also prone to changes in a near future

Worked on it for 1.5 years, used it in production for 5 months. No changes are planned, only enhancements.

@n1k0
Copy link
Member

n1k0 commented Sep 20, 2023

This branch has diverged much while we'r epretty much happy with Parcel atm, so I'm gonna close this one.

@n1k0 n1k0 closed this Sep 20, 2023
@n1k0 n1k0 deleted the use-elm-watch branch April 16, 2024 11:59
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.

3 participants