sitevision / sitevision-apps

Create SiteVision WebApps and RESTApps
15 stars 17 forks source link

Spread initialState in index #111

Open oskarakerlund-consid opened 1 year ago

oskarakerlund-consid commented 1 year ago

Så man slipper repetera properties i agnosticRender

Relaterat till #110

oskarakerlund-consid commented 1 year ago

Vad säger du om denna och #110 @albinohrn ? :)

albinohrn commented 1 year ago

Hej, jag våndas lite över den här frågan och därför har svar uteblivit. Sorry! Jag förstår att det kan vara smidigt för er att ha det, men samtidigt är jag lite osäker på om det är något vi vill predika som default.

Det är viktigt att man förstår vad som händer annars finns det risk att man skickar mer data än nödvändigt till klienten. Därför har vi inte velat ändra till detta som default, bättre att man är tvungen att bestämma vad som skickas med till klienten explicit än att nånting råkar åka med som inte var meningen. Man får ingen varning i TS t ex om man skickar in för mycket i sitt initialState.

const intialState = {
  name,
  message,
  irrelevantProperty: 'value'
};

// Förutsatt att App är den komponenten vi har i boilerplaten
// så kommer inte detta att ge några varningar.
renderToString(<App {...initialState} />);

// Hade jag däremot gjort följande så hade jag fått varningar från TypeScript eftersom att irrelevantProperty inte hanteras av APP
renderToString(<App name={initialState.name} message={initialState.message} irrelevantProperty="value" />);

Däremot om man vet vad man gör, som ni, och tycker att det är smidigt då finns möjligheten att ändra, men vi lutar fortfarande åt att det är bättre att vara explicit som default.

oskarakerlund-consid commented 1 year ago

Tack för utförligt svar @albinohrn - förstår hur du tänker! Det verkar som att båda alternativen har sina fördelar och nackdelar men då är det nog bäst att välja säkerhet framför smidighet. Det ska dock till en riktig tabbe om man råkar sätta något osäkert/onödigt i initialState som man inte vill leverera till App.