add support for workspaces #86
Conversation
client/cli/lib/start.ts
Outdated
|
|
||
| app | ||
| .route('/:component') | ||
| .all((req, res, next) => { |
There was a problem hiding this comment.
this check can be skipped if you create the routes in a forEach loop on the index...
There was a problem hiding this comment.
It also adds the component to the request
There was a problem hiding this comment.
I saw that but if you work in the closure of a foreach you get the component's name from the parameters so you don't need to add the name...
index.forEach(nameOfComponents => app.route(/nameOfComponents).get((req, res) => {
res.setHeader('Access-Control-Expose-Headers', 'ETag');
const component = nameOfComponent;
res.sendFile(resolve(process.cwd(), component.dir, 'dist', component.main));
}));
Removes validation code... Does it make sense?
There was a problem hiding this comment.
but then you don't get the logger warning if you're trying to get a non existing component
client/react/lib/index.tsx
Outdated
|
|
||
| console.warn(`failed getting component ${name} from dev server`, err); | ||
|
|
||
| if (!devMode) { |
There was a problem hiding this comment.
How can I get to this code path with !devMode?
There was a problem hiding this comment.
So the approach here is that if I have devMode === false I'm in global dev mode... mode... and so I should treat all components in dev mode? I would prefer to be more explicit... how about returning from the provider some property that says devModeOverride or globalDevMode?
There was a problem hiding this comment.
if you're in global dev mode, you get a dev client from the provider.
if the component is explicitly using dev mode we won't try getting the component from remote server and render the fallback
if the component is not explicitly using dev mode we try getting the component from remote server, and if that fails too render fallback
There was a problem hiding this comment.
also added - if you explicitly set devMode: false on a component, it will not use global devMode
|
I know there are a lot of comments but this is a huge (and awesome) new capability! |
nataly87s
force-pushed
the
workspaces
branch
2 times, most recently
from
d8c0fd0
to
2bcbc10
Compare
|
Just a general comment but this looks awesome and we are very much looking forward to using this enhancement in Soluto Nashville! 💯 |
|
@eladav I added support for config files per package when using workspaces |
|
@eladav @EladBezalel what's up? any comments? requests? something? |
| @@ -0,0 +1,65 @@ | |||
| import { resolve } from 'path'; | |||
| import { cosmiconfig, defaultLoaders } from 'cosmiconfig'; | |||
| interval?: number; | ||
| urlOverride?: string; | ||
| } | ||
|
|
||
| export class DynamicoDevClient extends DynamicoClient { |
There was a problem hiding this comment.
not sure I understand why is there a need to remove the inheritance here? wouldn't it work just the same?
There was a problem hiding this comment.
Because the api is different so inheritance doesn't make sense.
You said you used inheritance because you didn't want to duplicate code/logic, and I accomplished that by just using the regular client internally
There was a problem hiding this comment.
This is a good change (and also you improved other parts of the code)... Talk to us when you want to start using this and we'll support you. I'm not sure I understand the reason to remove the inheritance in from DevClient but we can refactor this at a later time. as soon as you'll merge we'll publish this version.
|
@eladav @EladBezalel I don't have permissions to merge |
#85