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

chore(frontend): remix v2 upgrade #2349

Merged
merged 44 commits into from
Jan 26, 2024
Merged

chore(frontend): remix v2 upgrade #2349

merged 44 commits into from
Jan 26, 2024

Conversation

BlairCurrey
Copy link
Contributor

@BlairCurrey BlairCurrey commented Jan 18, 2024

Changes proposed in this pull request

  • upgrades remix to v2 which includes:
    • refactoring ErrorBoundary (now handles what was previously seperate ErrorBoundary and CaughtBoundary)
    • updates MetaFunction args
    • specifies cjs as module type (default switched to esm)
    • renames some types whose names were changed
    • renames routes in MASE to conform to new route nesting pattern (denoted by . rather than sub directory)
  • adds typecheck command to package.json and run in ci (as well as fixing a pre-existing type error)

Context

fixes #1464
renovate pr #1886

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Postman collection updated

Copy link

netlify bot commented Jan 18, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 455ebb8
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65b2b7564222980008691228

@github-actions github-actions bot added the pkg: frontend Changes in the frontend package. label Jan 18, 2024
- upgrades all remix package versions
- fixes breaking v2 change
- removes v2 future flags
- removes skipLibCheck from tsconfig
@github-actions github-actions bot added the type: ci Changes to the CI label Jan 19, 2024
@BlairCurrey BlairCurrey marked this pull request as ready for review January 19, 2024 16:41
@BlairCurrey BlairCurrey changed the title chore(frontend): prepare for remix v2 chore(frontend): remix v2 upgrade Jan 19, 2024
@BlairCurrey
Copy link
Contributor Author

I noticed form the renovate pr that we're also using remix in the MASE. going to update that as well.

@github-actions github-actions bot added pkg: documentation Changes in the documentation package. type: documentation (archived) Improvements or additions to documentation labels Jan 25, 2024
Comment on lines +24 to +26
secure: process.env.ENABLE_INSECURE_MESSAGE_COOKIE
? !parseBool(process.env.ENABLE_INSECURE_MESSAGE_COOKIE)
: process.env.NODE_ENV === 'production'
Copy link
Contributor Author

@BlairCurrey BlairCurrey Jan 25, 2024

Choose a reason for hiding this comment

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

Added so that we dont have to run in development mode to set this to insecure. Basically, you can pass in ENABLE_INSECURE_MESSAGE_COOKIE=true to set this to false otherwise it uses NODE_ENV like we currently do.

this means it's secure by default for production use and when running in dev mode we dont need to set ENABLE_INSECURE_MESSAGE_COOKIE

for good measure here's a state table:

ENABLE_INSECURE_MESSAGE_COOKIE NODE_ENV Secure
true any false
false any true
undefined 'production' true
undefined 'development' false
undefined undefined true

@BlairCurrey BlairCurrey requested a review from mkurapov January 25, 2024 02:10

CMD ["sh", "-c", "NODE_ENV=production ENABLE_INSECURE_MESSAGE_COOKIE=true ./node_modules/.bin/remix-serve ./build/index.js"]
Copy link
Member

@raducristianpopa raducristianpopa Jan 25, 2024

Choose a reason for hiding this comment

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

I think the ENABLE_INSECURE_MESSAGE_COOKIE should be passed as a build argument in the compose file. ENABLE_INSECURE_MESSAGE_COOKIE will be true when starting the frontend application every time (for everyone that is going to use Rafiki's frontend image), since we use this specific Dockerfile to build the image.

https://docs.docker.com/compose/compose-file/compose-file-v3/#args

Suggested change
CMD ["sh", "-c", "NODE_ENV=production ENABLE_INSECURE_MESSAGE_COOKIE=true ./node_modules/.bin/remix-serve ./build/index.js"]
ARG ENABLE_INSECURE_MESSAGE_COOKIE=false # default value if the argument is not present at build time
ENV NODE_ENV=production
ENV ENABLE_INSECURE_MESSAGE_COOKIE=$ENABLE_INSECURE_MESSAGE_COOKIE
CMD ["sh", "-c", "./node_modules/.bin/remix-serve ./build/index.js"]

I experimented with this method and it appears to be effective for me. I would greatly appreciate any additional feedback on it to see if it works for everyone since I'm not entirely sure about this approach.

Copy link
Member

@raducristianpopa raducristianpopa Jan 25, 2024

Choose a reason for hiding this comment

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

The above approach might not even be necessary, we can just get rid of the inline environment variables and specify the value for ENABLE_INSECURE_MESSAGE_COOKIE in the compose files. Setting the value of the environment variable before a command, it will replace the value that was previously set in the compose file.

Suggested change
CMD ["sh", "-c", "NODE_ENV=production ENABLE_INSECURE_MESSAGE_COOKIE=true ./node_modules/.bin/remix-serve ./build/index.js"]
CMD ["sh", "-c", "./node_modules/.bin/remix-serve ./build/index.js"]

Copy link
Contributor Author

@BlairCurrey BlairCurrey Jan 25, 2024

Choose a reason for hiding this comment

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

Good point - I will move it to the compose.

I can also remove NODE_ENV=production from the frontend dockerfile but not necessarily the MASE because those are set to development by compose. I take the point that we shouldn't set it here. Maybe I can remove NODE_ENV=development from the compose for the MASE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the development NODE_ENV from the MASE's in the docker compose and removed the inline env var from the dockerfile. MASE's start fine and look normal - audited for any NODE_ENV checks in the MASE and there are none so there seems to be no problem with that. b35dc09

@@ -31,7 +31,7 @@ servers:
- url: 'https://ilp.rafiki.money'
description: 'Server for wallet address subresources (ie https://ilp.rafiki.money/alice)'
- url: 'https://ilp.rafiki.money/.well-known/pay'
description: 'Server for when the wallet address has no pathname (ie https://ilp.rafiki.money)'
description: 'Server for when Payment Pointer has no pathname (ie https://ilp.rafiki.money)'
Copy link
Contributor

Choose a reason for hiding this comment

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

is this an old version maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think just need to remerge main in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch... lost it on a reverted merge. will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

packages/frontend/package.json Outdated Show resolved Hide resolved
sabineschaller and others added 3 commits January 25, 2024 13:40
* feat(backend): upgrade to open payments 6.5

* refactor: one singleton for openApi specs
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@github-actions github-actions bot removed type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Jan 25, 2024
@BlairCurrey BlairCurrey requested a review from mkurapov January 25, 2024 19:23
@BlairCurrey BlairCurrey merged commit 7f00fc0 into main Jan 26, 2024
22 checks passed
@BlairCurrey BlairCurrey deleted the bc/1464/remix-v2-updates branch January 26, 2024 14:53
beniaminmunteanu pushed a commit that referenced this pull request Feb 2, 2024
* chore(frontend): update error boundaries to v2

* chore(frontend): update meta to v2

* chore(frontend): update headers to v2

* chore(frontend): update form method to v2

* chore(frontend): update to v2 dev server

* chore(frontend): update server module format to v2

* feat(frontend): typecheck cmd

* fix(frontend): type only import

* feat(frontend): ugprade remix to v2

- upgrades all remix package versions
- fixes breaking v2 change
- removes v2 future flags
- removes skipLibCheck from tsconfig

* feat: add frontend typecheck to ci

* chore(MASE): upgrade routes to remix v2

* chore(MASE): upgrade module format to v2

* chore(MASE): update to v2 dev server

* chore(MASE): update headers to v2

* chore(MASE): update form method to v2

* chore(MASE): update meta to v2

* chore(MASE): update error/catch boundary to v2

* feat(MASE): upgrade remix to v2

* fix(MASE): ts errors

* feat(MASE): add typecheck cmd to ci

* fix(frontend, MASE): remix server start

* fix(MASE): eslint

* refactor(MASE): parseQueryString to URLSearchParams

* Update .github/workflows/lint_test_build.yml

Co-authored-by: Radu-Cristian Popa <radu.popa@breakpointit.eu>

* fix(frontend,MASE): update entry.server request handlers

* chore(MASE): format

* chore(MASE): rm unused quotes route

* Revert "Merge branch 'main' into bc/1464/remix-v2-updates"

This reverts commit f7149aa, reversing
changes made to 41673bb.

* chore: cherry pick merge commit after reverting in f7149aa

chore: upgrade to typescript 5 (#2345)

* feat: upgrade ts to 5.0

* fix(auth): ts2365 build error

fixes Forbidden Implicit Coercions in Relational Operators

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#forbidden-implicit-coercions-in-relational-operators

* chore(auth): rm unecessary type assertion

* fix(backend): ts2365 build error

fixes Forbidden Implicit Coercions in Relational Operators

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#forbidden-implicit-coercions-in-relational-operators

* chore: rm default tsconfig option

forceConsistentCasingInFileNames defaults to true in ts 5.0

* chore: update package versions

* chore: rm unused optoin

* chore: rm typescript dev dep from pacakges

* fix(frontend,MASE): mobile view

* fix(frontend): no flash on mobile

* refactor: rework new cookie secure env var

* fix: move env var declaration from dockerfile to compose

* feat(backend): upgrade Rafiki to open payments package v6.5 (#2337)

* feat(backend): upgrade to open payments 6.5

* refactor: one singleton for openApi specs

* chore(deps): update dependency autoprefixer to ^10.4.17 (#2352)

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

* chore: update lockfile

* chore: rm extra space

* chore: rm unnecessary eslint ignore

---------

Co-authored-by: Radu-Cristian Popa <radu.popa@breakpointit.eu>
Co-authored-by: Sabine Schaller <sabine@interledger.org>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
beniaminmunteanu pushed a commit that referenced this pull request Feb 8, 2024
* chore(frontend): update error boundaries to v2

* chore(frontend): update meta to v2

* chore(frontend): update headers to v2

* chore(frontend): update form method to v2

* chore(frontend): update to v2 dev server

* chore(frontend): update server module format to v2

* feat(frontend): typecheck cmd

* fix(frontend): type only import

* feat(frontend): ugprade remix to v2

- upgrades all remix package versions
- fixes breaking v2 change
- removes v2 future flags
- removes skipLibCheck from tsconfig

* feat: add frontend typecheck to ci

* chore(MASE): upgrade routes to remix v2

* chore(MASE): upgrade module format to v2

* chore(MASE): update to v2 dev server

* chore(MASE): update headers to v2

* chore(MASE): update form method to v2

* chore(MASE): update meta to v2

* chore(MASE): update error/catch boundary to v2

* feat(MASE): upgrade remix to v2

* fix(MASE): ts errors

* feat(MASE): add typecheck cmd to ci

* fix(frontend, MASE): remix server start

* fix(MASE): eslint

* refactor(MASE): parseQueryString to URLSearchParams

* Update .github/workflows/lint_test_build.yml

Co-authored-by: Radu-Cristian Popa <radu.popa@breakpointit.eu>

* fix(frontend,MASE): update entry.server request handlers

* chore(MASE): format

* chore(MASE): rm unused quotes route

* Revert "Merge branch 'main' into bc/1464/remix-v2-updates"

This reverts commit f7149aa, reversing
changes made to 41673bb.

* chore: cherry pick merge commit after reverting in f7149aa

chore: upgrade to typescript 5 (#2345)

* feat: upgrade ts to 5.0

* fix(auth): ts2365 build error

fixes Forbidden Implicit Coercions in Relational Operators

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#forbidden-implicit-coercions-in-relational-operators

* chore(auth): rm unecessary type assertion

* fix(backend): ts2365 build error

fixes Forbidden Implicit Coercions in Relational Operators

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#forbidden-implicit-coercions-in-relational-operators

* chore: rm default tsconfig option

forceConsistentCasingInFileNames defaults to true in ts 5.0

* chore: update package versions

* chore: rm unused optoin

* chore: rm typescript dev dep from pacakges

* fix(frontend,MASE): mobile view

* fix(frontend): no flash on mobile

* refactor: rework new cookie secure env var

* fix: move env var declaration from dockerfile to compose

* feat(backend): upgrade Rafiki to open payments package v6.5 (#2337)

* feat(backend): upgrade to open payments 6.5

* refactor: one singleton for openApi specs

* chore(deps): update dependency autoprefixer to ^10.4.17 (#2352)

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

* chore: update lockfile

* chore: rm extra space

* chore: rm unnecessary eslint ignore

---------

Co-authored-by: Radu-Cristian Popa <radu.popa@breakpointit.eu>
Co-authored-by: Sabine Schaller <sabine@interledger.org>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
njlie pushed a commit that referenced this pull request Feb 29, 2024
* chore(frontend): update error boundaries to v2

* chore(frontend): update meta to v2

* chore(frontend): update headers to v2

* chore(frontend): update form method to v2

* chore(frontend): update to v2 dev server

* chore(frontend): update server module format to v2

* feat(frontend): typecheck cmd

* fix(frontend): type only import

* feat(frontend): ugprade remix to v2

- upgrades all remix package versions
- fixes breaking v2 change
- removes v2 future flags
- removes skipLibCheck from tsconfig

* feat: add frontend typecheck to ci

* chore(MASE): upgrade routes to remix v2

* chore(MASE): upgrade module format to v2

* chore(MASE): update to v2 dev server

* chore(MASE): update headers to v2

* chore(MASE): update form method to v2

* chore(MASE): update meta to v2

* chore(MASE): update error/catch boundary to v2

* feat(MASE): upgrade remix to v2

* fix(MASE): ts errors

* feat(MASE): add typecheck cmd to ci

* fix(frontend, MASE): remix server start

* fix(MASE): eslint

* refactor(MASE): parseQueryString to URLSearchParams

* Update .github/workflows/lint_test_build.yml

Co-authored-by: Radu-Cristian Popa <radu.popa@breakpointit.eu>

* fix(frontend,MASE): update entry.server request handlers

* chore(MASE): format

* chore(MASE): rm unused quotes route

* Revert "Merge branch 'main' into bc/1464/remix-v2-updates"

This reverts commit f7149aa, reversing
changes made to 41673bb.

* chore: cherry pick merge commit after reverting in f7149aa

chore: upgrade to typescript 5 (#2345)

* feat: upgrade ts to 5.0

* fix(auth): ts2365 build error

fixes Forbidden Implicit Coercions in Relational Operators

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#forbidden-implicit-coercions-in-relational-operators

* chore(auth): rm unecessary type assertion

* fix(backend): ts2365 build error

fixes Forbidden Implicit Coercions in Relational Operators

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#forbidden-implicit-coercions-in-relational-operators

* chore: rm default tsconfig option

forceConsistentCasingInFileNames defaults to true in ts 5.0

* chore: update package versions

* chore: rm unused optoin

* chore: rm typescript dev dep from pacakges

* fix(frontend,MASE): mobile view

* fix(frontend): no flash on mobile

* refactor: rework new cookie secure env var

* fix: move env var declaration from dockerfile to compose

* feat(backend): upgrade Rafiki to open payments package v6.5 (#2337)

* feat(backend): upgrade to open payments 6.5

* refactor: one singleton for openApi specs

* chore(deps): update dependency autoprefixer to ^10.4.17 (#2352)

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

* chore: update lockfile

* chore: rm extra space

* chore: rm unnecessary eslint ignore

---------

Co-authored-by: Radu-Cristian Popa <radu.popa@breakpointit.eu>
Co-authored-by: Sabine Schaller <sabine@interledger.org>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: documentation Changes in the documentation package. pkg: frontend Changes in the frontend package. pkg: mock-ase type: ci Changes to the CI type: documentation (archived) Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare for Remix future changes
5 participants