[Work in progress] Separation of concerns using Surplus #116
Conversation
Vinnl
left a comment
Vinnl
left a comment
There was a problem hiding this comment.
Overall I really like the direction this is going in! It's a clear improvement over what we have now, in my eyes. I've added some food for thought inline that we can discuss.
The one major point, which I'm guessing you're probably already expecting, concerns the addition of Surplus. While I generally like its concepts, it comes with clear longevity concerns, which means we should be prepared to fork and maintain it eventually - and I'm not sure whether we have the capacity to do that, and whether that'd be worth it.
Thus, I think the primary thing I'd like a stronger rationale for, is: why not go with e.g. React, especially considering that that would allow us to reuse and dogfood the SDK?
| @@ -0,0 +1,3 @@ | |||
| declare module 'marked' { | |||
There was a problem hiding this comment.
Oh, there's a definition already, didn't see that - should make use of that instead.
| @@ -0,0 +1,3 @@ | |||
| declare module 'marked' { | |||
|
|
||
| declare module 'solid-ui' { | ||
| export type SolidUi = any | ||
| export type SolidUi = { |
There was a problem hiding this comment.
This PR should make this file redundant: https://github.com/solid/solid-ui/pull/70
| }, | ||
| module: { | ||
| rules: [ | ||
| { test: /\.tsx$/, loader: 'surplus-loader!ts-loader' }, |
There was a problem hiding this comment.
Babel can load TypeScript files too, so I don't think there's a need to introduce ts-loader here.
| "jest": "^24.8.0", | ||
| "surplus-loader": "^0.5.0", | ||
| "ts-jest": "^24.0.2", | ||
| "ts-loader": "^6.0.2", |
There was a problem hiding this comment.
As said below, I think ts-loader shouldn't be necessary.
| import { TrustedApplication } from './trustedApplications.models' | ||
| import { getStatementsToAdd, getStatementsToDelete } from './trustedApplications.service' | ||
|
|
||
| const { authn, ns, store } = solidUi |
There was a problem hiding this comment.
I think it'd be good to move away from the SolidUI Singleton, insofar as possible, i.e. by limiting access to it to trustedApplications/trustedApplicationsPane, and have that pass the required data (e.g. the current session) as a parameter to other modules that need it.
There was a problem hiding this comment.
I'm thinking along this lines as well, but didn't want to revolutionize the code base all at once =P Probably need a migration path for this, but could of course introduce that in these conventions
| })) | ||
| } | ||
|
|
||
| static createNew (_subject: NamedNode): TrustedApplication { |
There was a problem hiding this comment.
I'm wondering whether a Factory might be overengineering this. As an alternative, we could also just define an interface using something like this:
export interface TrustedApplication {
origin: DataSignal<string>
subect: DataSignal<string>
modes: Mode[]
}
export const fromSubject: (subject: NamedNode) => TrustedApplication = /* ... */
export const fromNamedNodes (subject: NamedNode, origin: NamedNode | null, modes: NamedNode[]) => TrustedApplication = /* ... */Then no private constructor is needed, nor copy (which would just be const copiedApp = {...originalApp}), and a simple object would suffice.
| const { S } = Surplus | ||
| const { widgets } = solidUi | ||
|
|
||
| export const TrustedApplicationsView = (controller: TrustedApplicationsController) => |
There was a problem hiding this comment.
It is not directly clear to me that the separation of the view and the controller offers much value here - they seem pretty tightly coupled, with the controller's methods being closely related to what the view is doing. Would it not be more readable to have that logic co-located with the view that is influenced by it?
Rebased revamp on master
Taking surplus for a spin - seems very easy to make use of, I like it!
|
This relates to #72 |
|
We've decided to use React for this purpose going forward. |
Trying out new conventions using Surplus
This library allows us to write markup with JSX and handles internal state.
I've split code into controllers, views, models, and services.
This PR is to showcase the changes I've done, should not be merged before a broader discussion.