Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

npm run dist does not work but npm run build + npm run zip does #76

Closed
msramalho opened this issue Aug 25, 2020 · 2 comments · Fixed by #88
Closed

npm run dist does not work but npm run build + npm run zip does #76

msramalho opened this issue Aug 25, 2020 · 2 comments · Fixed by #88
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@msramalho
Copy link
Owner

I was unable to understand why, but the zip command only works properly if executed individually, otherwise only the manifest file is copied to the zipped versions.

@msramalho msramalho added bug Something isn't working help wanted Extra attention is needed labels Aug 25, 2020
@fabiodrg
Copy link
Collaborator

fabiodrg commented Mar 2, 2021

It seems the problem has to do with pipes being async. By the time the mergeAll returns, not all pipes are completed. Experiment:

function mergeAll(done) {
    pipe(...);
    pipe(...);
    ...
    setTimeout(function () {
        done()
    }, 5000);
}

The delay of 5 seconds before done() ensures all buffers and streams flushed (i.e. the destination files are created) before the task zip.
Not sure how to solve it, but will try to use Promises API.

@fabiodrg fabiodrg self-assigned this Mar 2, 2021
@fabiodrg
Copy link
Collaborator

fabiodrg commented Mar 2, 2021

After inspecting in more detail, I think the most sensible option is to separate each move command (coping files from src/ to build/) to individual Gulp tasks.

I didn't find a method that lets return multiple streams. As per Gulp, a task can only return one stream. Moreover, I suspect Gulp uses synchronous Node.js streams, which don't allow me to use a Promisse.all() to handle all promises at once. Therefore, I would have to add event handlers to capture the stream closing/finishing, increment some counter, and when the counter is N, then all streams are flushed and the task can conclude.

const t1 = pipe('./src/css/**/*', `./build/${target}/css`);
t1.on('finish', () => console.log('Finish'));
...

That versus individual tasks per copy/move operation that can run in parallel, I think the latter is more sensible to be compliant with Gulp, faster to code and easier to read.

fabiodrg added a commit that referenced this issue Mar 2, 2021
Fixes bug in the 'dist' npm command where the final Zip files did not
contain all the necessary files due to async code. See #76
@fabiodrg fabiodrg added this to Done in v4.2 Mar 28, 2022
@fabiodrg fabiodrg removed this from Done in v4.2 Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants