Update Elixir and Phoenix and all deps #395

lleger · 2022-08-06T02:33:21Z

Look over your code one last time.
Test all functionality you expect to be QA'd. Don’t forget to QA user interface changes in the browser.
Describe what changes this PR includes.
Post screenshots for design review if applicable.
Fill in QA Notes listing clear, ordered steps for testers.

Description

Also updates generated files from new Phoenix project. I updated this
manually comparing a fresh Phoenix 1.6.11. I also used the PhoenixDiff
project to double check.

Closes [sc-20388].

Screenshots

N/A

QA Notes

PR Author Browser Info: N/A

**QA Tester Browser Info:**

## Environment Setup

- [ ] `asdf install`
- [ ] `mix deps.get`

## Testing Steps

- [ ] Tests should pass
- [ ] App should boot

## Notes

- [ ] [Tester should add any comments, questions, or concerns that pop up during QA]

shortcut-integration · 2022-08-06T02:33:24Z

This pull request has been linked to Shortcut Story #20388: Update versions and add any improvements from newer Phoenix versions.

lleger · 2022-08-06T02:34:40Z

Dockerfile

-COPY config config
-COPY mix.* ./
-RUN mix do deps.get --only=$MIX_ENV, deps.compile
+COPY mix.exs mix.lock ./


The changes here are mostly due to how Docker caches. Each line creates a new layer. Based on what needs to be recompiled, separating some steps and copying at different points helps maximize the Docker cache, which speeds up the build.

These are picked up from the new Phoenix default Dockerfile.

lleger · 2022-08-06T02:36:07Z

assets/package.json

-    "deploy": "webpack --mode production",
-    "watch": "webpack --mode development --watch"
-  },
+  "scripts": {},


@ngscheurich Any thoughts on how to write a script here that mirrors mix lint I added to combine all the JS linters/formatters we have in CI into one command to run locally?

@lleger Yep! I'm going to add a commit for this.

lleger · 2022-08-06T02:37:11Z

lib/phoenix_starter/users/user_notifier.ex

-  def deliver_confirmation_instructions(user, url, true) do
-    %{email: "confirmation_instructions", user_id: user.id, url: url}
-    |> UserEmailWorker.new()
-    |> Oban.insert()


The Swoosh README (written by Jose) recommends not doing this unless you have a business case for it, so by default I'm removing this code. It can be added later if the specific app requires it.

lleger · 2022-08-06T02:38:00Z

lib/phoenix_starter_web/channels/user_socket.ex

@@ -1,35 +0,0 @@
-defmodule PhoenixStarterWeb.UserSocket do


These are just not generated by default anymore and instead are in a generator.

Also updates generated files from new Phoenix project. I updated this manually comparing a fresh Phoenix 1.6.11. I also used the [PhoenixDiff] project to double check. [phoenixdiff]: https://www.phoenixdiff.org/?source=1.5.6&target=1.6.11

lleger · 2022-08-06T04:52:16Z

.github/workflows/ci.yml

@@ -88,10 +88,6 @@ jobs:
        working-directory: ./assets
      - run: npx eslint js --ext .js,.jsx,.ts,.tsx
        working-directory: ./assets
-      - run: npm run deploy


This was running web pack, which we no longer need to do.

@ngscheurich Is there a lint we can run for Typescript to ensure our types pass? I think this deploy step was added to get the TS checks.

Unfortunately, esbuild does not do any type-checking so we'll still need to depend on the typescript package and run npx tsc --noEmit in CI.

lleger · 2022-08-06T04:53:06Z

.github/workflows/ci.yml

@@ -174,9 +170,7 @@ jobs:

      - name: Create PLTs
        if: steps.plt-cache.outputs.cache-hit != 'true'
-        run: |
-          mkdir -p priv/plts


No longer needed, Dialyxir does this automatically now.

jeremyjh/dialyxir#368

lleger · 2022-08-06T04:53:55Z

mix.exs

@@ -69,17 +71,24 @@ defmodule PhoenixStarter.MixProject do
    [
      ignore_warnings: ".dialyzer_ignore.exs",
      plt_file: {:no_warn, "priv/plts/dialyzer.plt"},
-      flags: [:error_handling, :race_conditions, :underspecs, :unknown],


race_conditions dropped in OTP 25

erlang/otp@74c65fb

ngscheurich

Looks great.

ngscheurich · 2022-08-09T01:52:09Z

.github/workflows/ci.yml

@@ -88,10 +88,6 @@ jobs:
        working-directory: ./assets
      - run: npx eslint js --ext .js,.jsx,.ts,.tsx
        working-directory: ./assets
-      - run: npm run deploy


Unfortunately, esbuild does not do any type-checking so we'll still need to depend on the typescript package and run npx tsc --noEmit in CI.

ngscheurich · 2022-08-09T01:55:21Z

assets/package.json

-    "deploy": "webpack --mode production",
-    "watch": "webpack --mode development --watch"
-  },
+  "scripts": {},


@lleger Yep! I'm going to add a commit for this.

ngscheurich · 2022-08-09T02:01:59Z

lib/phoenix_starter_web/live/user_settings/index.html.heex

-  <%= live_patch "Update profile", to: Routes.user_settings_path(@socket, :profile) %>
-  <%= live_patch "Update email", to: Routes.user_settings_path(@socket, :email) %>
-  <%= live_patch "Update password", to: Routes.user_settings_path(@socket, :password) %>
+  <%= live_patch("Update profile", to: Routes.user_settings_path(@socket, :profile)) %>


I guess the Phoenix.LiveView.HTMLFormatter added these parens all over the place? I don't like it but I suppose we should abide by the formatter.

ngscheurich · 2022-08-09T02:24:51Z

.github/workflows/ci.yml

-        env:
-          NODE_ENV: production
-        working-directory: ./assets
+      - run: npm run ci


@lleger Replaced the individual run steps with a single NPM script.

ngscheurich · 2022-08-09T02:27:36Z

@lleger I added some NPM scripts for linting, formatting, and type checking. These surfaced some TypeScript errors which I'll have a look at tomorrow.

ngscheurich · 2022-08-09T02:25:23Z

assets/package.json

+    "ci": "npm run lint && npm run format:check && npm run type-check",
+    "format": "prettier --write .",
+    "format:check": "prettier --check .",
+    "lint": "npm run lint:css && npm run lint:js",
+    "lint:css": "stylelint css/**/*.css",
+    "lint:css:fix": "stylelint css/**/*.css --fix",
+    "lint:fix": "npm run lint:css:fix && npm run lint:js:fix",
+    "lint:js": "eslint . --ext .js,.jsx,.ts,.tsx",
+    "lint:js:fix": "eslint . --ext .js,.jsx,.ts,.tsx --fix",
+    "type-check": "tsc --noEmit"


@lleger How do these scripts look to you?

ngscheurich · 2022-08-09T02:26:25Z

assets/tsconfig.json

@@ -6,6 +6,7 @@
    "module": "es6",
    "moduleResolution": "node",
    "noImplicitAny": true,
+    "skipLibCheck": true,


This is standard in TypeScript projects initialized with tsc --init and now we don't have to remember to include it as a CLI arg.

This aligns us with current best practices documented at https://www.typescriptlang.org/tsconfig.

ngscheurich · 2022-08-09T16:31:25Z

assets/tsconfig.json

+    /* Language and Environment */
+    "target": "ES6",
+    "lib": ["DOM", "ES2017"],
    "jsx": "react",
+
+    /* Modules */
    "module": "es6",
    "moduleResolution": "node",
-    "noImplicitAny": true,
+
+    /* Interop Constraints */
+    "esModuleInterop": true,
+
+    /* Type Checking */
    "strict": true,
-    "target": "es5"
+
+    /* Completeness */
+    "skipLibCheck": true


This aligns us with current best practices documented at https://www.typescriptlang.org/tsconfig.

custodian bot added the in-progress Work is ongoing or pending label Aug 6, 2022

lleger commented Aug 6, 2022

View reviewed changes

lleger force-pushed the logan/sc-20388/update-versions-and-add-any-improvements branch 3 times, most recently from b29b15d to a00917a Compare August 6, 2022 04:51

Update Elixir and Phoenix and all deps

5d0dc9d

Also updates generated files from new Phoenix project. I updated this manually comparing a fresh Phoenix 1.6.11. I also used the [PhoenixDiff] project to double check. [phoenixdiff]: https://www.phoenixdiff.org/?source=1.5.6&target=1.6.11

lleger force-pushed the logan/sc-20388/update-versions-and-add-any-improvements branch from a00917a to 5d0dc9d Compare August 6, 2022 04:57

lleger commented Aug 6, 2022

View reviewed changes

lleger marked this pull request as ready for review August 6, 2022 05:07

lleger requested a review from ngscheurich August 6, 2022 05:07

lleger added needs-review Work is finished; awaiting code review and removed in-progress Work is ongoing or pending labels Aug 6, 2022

This was referenced Aug 6, 2022

Add format_timestamp helper function #396

Draft

Update flash messages #397

Draft

ngscheurich approved these changes Aug 9, 2022

View reviewed changes

custodian bot added ready-to-merge Work is finished and reviewed; awaiting merge and removed needs-review Work is finished; awaiting code review labels Aug 9, 2022

ngscheurich added 4 commits August 8, 2022 21:12

Skip type checking all .d.ts files

e1b35f1

Add NPM scripts

72fe620

fixup! Add NPM scripts

b347eda

Ignore config files

d3d54a6

ngscheurich reviewed Aug 9, 2022

View reviewed changes

ngscheurich added in-progress Work is ongoing or pending and removed ready-to-merge Work is finished and reviewed; awaiting merge labels Aug 9, 2022

ngscheurich added 2 commits August 9, 2022 11:11

fixup! fixup! Add NPM scripts

71c6cbe

Update TypeScript configuration

3d24f78

This aligns us with current best practices documented at https://www.typescriptlang.org/tsconfig.

ngscheurich reviewed Aug 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Elixir and Phoenix and all deps #395

Update Elixir and Phoenix and all deps #395

lleger commented Aug 6, 2022 •

edited

Loading

shortcut-integration bot commented Aug 6, 2022

lleger Aug 6, 2022

lleger Aug 6, 2022

ngscheurich Aug 9, 2022

lleger Aug 6, 2022

lleger Aug 6, 2022

lleger Aug 6, 2022

lleger Aug 6, 2022

ngscheurich Aug 9, 2022

lleger Aug 6, 2022

lleger Aug 6, 2022

lleger Aug 6, 2022

lleger Aug 6, 2022

ngscheurich left a comment

ngscheurich Aug 9, 2022

ngscheurich Aug 9, 2022

ngscheurich Aug 9, 2022

ngscheurich Aug 9, 2022 •

edited

Loading

ngscheurich commented Aug 9, 2022

ngscheurich Aug 9, 2022

ngscheurich Aug 9, 2022

ngscheurich Aug 9, 2022

Update Elixir and Phoenix and all deps #395

Are you sure you want to change the base?

Update Elixir and Phoenix and all deps #395

Conversation

lleger commented Aug 6, 2022 • edited Loading

Description

Screenshots

QA Notes

shortcut-integration bot commented Aug 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngscheurich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngscheurich Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

ngscheurich commented Aug 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lleger commented Aug 6, 2022 •

edited

Loading

ngscheurich Aug 9, 2022 •

edited

Loading