fix: remove components respect dependencies Close #16 #17
Conversation
|
@yugasun great thanks for this PR. Can you explain in more detail, why is that needed, and why the |
|
@medikoo Sorry for the mistake, change |
|
Thank you @yugasun still can you explain in more detail why is that needed? (there's no description provided, and I don't understand intention behind this PR) |
utils.js
Outdated
| @@ -326,7 +334,7 @@ const syncState = async (allComponents, instance) => { | |||
| await metric.publish() | |||
| } | |||
|
|
|||
| promises.push(fn()) | |||
| await fn() | |||
There was a problem hiding this comment.
I think it'll be cleaner (and less confusing for one who may read this code later) to just remove the function wrap in such case, and remove further await Promise.all()
| this.context.status('Removing') | ||
|
|
||
| this.context.debug('Flushing template state and removing all components.') | ||
| await syncState({}, this) |
There was a problem hiding this comment.
Why syncState({}, this) is not enough to properly remove components?
There was a problem hiding this comment.
Because syncState() just remove all components at the same time, not analysis the dependency tree. For some relative components, should remove the relative component firstly, otherwise it will fail to remove.
There was a problem hiding this comment.
Because syncState() just remove all components at the same time,
But you've fixed that in that PR here: https://github.com/serverless/template/pull/17/files/bc2003dd824351b04cc40cb3ff9f3c1e436985c5#diff-14ef994fa167d355bdaf9b44add54b87R337
So my question is why is that fix not enough (?)
No description provided.