Feature/meditor 937 upgrade and simplify meditor #69
Conversation
joncarlson
force-pushed
the
feature/MEDITOR-937-upgrade-and-simplify-meditor
branch
from
ca6dd50 to
942a581
Compare
docker-compose.override.yml
Outdated
There was a problem hiding this comment.
Docker has introduced "develop" parameters that only take effect when developing locally, making a separate override compose unnecessary
There was a problem hiding this comment.
Most of this is merging the two docker compose files together, noting where there are new items or updates
There was a problem hiding this comment.
I think I'll need your help to get this running. I must have something misconfigured, but a docker system prune + rebuild and a few minutes of searching around yielded nothing.
| - .env | ||
| ports: | ||
| - "127.0.0.1:4000:4000" | ||
| develop: |
There was a problem hiding this comment.
New develop section for the app, makes volume mounting unnecessary. If you do docker-compose up --watch or docker compose watch these take effect
There was a problem hiding this comment.
Auto-updated from ESLint upgrade
packages/app/auth/http.ts
Outdated
There was a problem hiding this comment.
NextAuth, the authentication library this PR introduces, has the concept of custom login providers. This is a custom provider to authenticate with Earthdata Login.
Generally this provider is responsible for:
- telling NextAuth what oAuth authorize url to hit
- how to get a token from EDL
- how to retrieve user information from EDL after auth
There was a problem hiding this comment.
Simplifying the user types to what we are actually using in the app
| router.pathname == '/' || | ||
| router.pathname == '/installation' | ||
|
|
||
| const App = ({ |
There was a problem hiding this comment.
Look at all that auth related code that we don't need anymore!
There was a problem hiding this comment.
NextAuth configuration, again, looks complicated at first glance so I'll try to comment where needed
| }, | ||
|
|
||
| // configure one or more authentication providers | ||
| providers: [ |
There was a problem hiding this comment.
Here's where we can add more providers as we go (Github, Google, Launchpad, etc.)
| ], | ||
|
|
||
| callbacks: { | ||
| async signIn(session) { |
There was a problem hiding this comment.
Since this PR makes mEditor stateless (no DB session), we still need a way to create a user account in the "users-urs" collection.
The notifier uses this collection to pull email addresses when sending emails.
This would be unnecessary if we could get those emails from EDL, but alas that API doesn't exist from what I can see
There was a problem hiding this comment.
This isn't super important to me or anything, but I'm not sure I understand why we specifically have a users-urs collection when we've just introduced OAuth providers as a concept. In a future improvement, would it theoretically be a good decision to drop the 'users-urs' collection and user the Users collection, since all OAuth providers are going to provide an email address as a part of the user identity?
There was a problem hiding this comment.
Some base Zod schemas as the old API relied heavily on passing the "model" and "document" as query params
There was a problem hiding this comment.
The "handlePublicationAcknowledgements" function was almost entirely copied from the legacy API
| ) | ||
| }) | ||
|
|
||
| test('handles primitive data types just like JSON.parse', () => { | ||
| const [stringError, string] = safeParseJSON('string') | ||
| expect(stringError).toMatchInlineSnapshot( | ||
| `[SyntaxError: Unexpected token s in JSON at position 0]` | ||
| `[SyntaxError: Unexpected token 's', "string" is not valid JSON]` |
There was a problem hiding this comment.
More descriptive errors :)
packages/app/utils/errors.ts
Outdated
There was a problem hiding this comment.
Error handling moved to "with-api-error-handler"
packages/legacy-api/.gitignore
Outdated
There was a problem hiding this comment.
Removed the legacy API
packages/proxy/.prettierrc
Outdated
There was a problem hiding this comment.
Removed the proxy
| return JSON.parse(localStorage.getItem(getLocalStorageKey(modelName, localId))) | ||
| return typeof localStorage !== 'undefined' | ||
| ? JSON.parse(localStorage.getItem(getLocalStorageKey(modelName, localId))) | ||
| : [] |
There was a problem hiding this comment.
Should the return type here should be an object literal, since the stored document is an object?
| @@ -1,3 +1,7 @@ | |||
| { | |||
| "extends": "next/core-web-vitals" | |||
| "extends": "next/core-web-vitals", | |||
| "rules": { | |||
There was a problem hiding this comment.
I think the flat file configuration was deprecated in ESLint 9. Here's what works for me:
- change the file name to
eslint.config.js - use the config (I left some stuff commented out that Next recommended, since we might not want it)
import { FlatCompat } from '@eslint/eslintrc'
const compat = new FlatCompat({
// import.meta.dirname is available after Node.js v20.11.0
baseDirectory: import.meta.dirname,
})
const eslintConfig = [
...compat.config({
// extends: ['next/core-web-vitals', 'next/typescript'],
extends: ['next/core-web-vitals'],
}),
{
rules: {
'react-hooks/exhaustive-deps': 'off',
'@next/next/no-html-link-for-pages': 1,
},
},
]
export default eslintConfigThere was a problem hiding this comment.
I think I'll need your help to get this running. I must have something misconfigured, but a docker system prune + rebuild and a few minutes of searching around yielded nothing.
|
Oh, one more thought I had this morning. Do you want to update the |
Oh yes that's a good idea. Updating it |
| //? the design makes the login button appear in a modal dialog however if we use a React-Bootstrap modal, the modal will not hydrate correctly and will generate an error as | ||
| //? the modal is not present on the server side. Instead we'll use the HTML directly | ||
| return ( | ||
| <div |
joncarlson
deleted the
feature/MEDITOR-937-upgrade-and-simplify-meditor
branch
This PR moves us a bit closer to being able to go cloud native and also relieves some of the maintenance burden by simplifying mEditors stack. A few changes here: