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

No files are written: mkdir@1.0.0 broke mkdir-promise as such writeFile processor broke #287

Closed
dherges opened this issue Jan 29, 2020 · 6 comments · Fixed by #288
Closed

Comments

@dherges
Copy link
Contributor

dherges commented Jan 29, 2020

mkdirp@1.0.0 changed implementation from callback to promisified api


mkdir-promise has "mkdirp": "*" dependency: https://github.com/ahmadnassri/mkdirp-promise/blob/master/package.json#L37


As such, the writeFile stopped working because the Promise never resolves.

https://github.com/angular/dgeni-packages/blob/master/base/services/writeFile.js#L1


I fixed it locally in a project workspace by overriding the factory with a custom implementation:

import * as mkdirp from 'mkdirp';
import * as fs from 'fs';
import * as path from 'canonical-path';

/**
 * mkdirp module changed implementation from callback to promise in 1.0.0
 *
 * As such, mkdir-promise stopped working.
 *
 * @link https://github.com/isaacs/node-mkdirp/blob/master/CHANGELOG.md
 */
export default function writeFile() {

  return function(file, content) {

    return new Promise((resolve, reject) => {
        mkdirp(path.dirname(file), undefined, (err, made) => {
          if (err === null) {
            resolve(made);
          } else {
            reject(err);
          }
        });
      }).then(() => new Promise((resolve, reject) => {
        fs.writeFile(file, content, function (err) {
          if (err) {
            reject(err);
          }
          resolve();
        });
      }));
  }
}
@dherges dherges changed the title mkdir@1.0.0 broke mkdir-promise as such writeFile processor broke No files are written: mkdir@1.0.0 broke mkdir-promise as such writeFile processor broke Jan 29, 2020
@dherges
Copy link
Contributor Author

dherges commented Jan 29, 2020

Thoughts...just thoughs...

  1. Please take this issue to twitter 🤣
  2. Ideally, mkdir-promise should mkdirp instanceof Promise ? mkdir : promisify(mkdirp)... 🤔
  3. It could be worth adding a few promising lines of code for a Promise that's actually a Promise 🐆

@petebacondarwin
Copy link
Member

How about we just update dgeni-packages to using mkdirp@1.0 and stop using mkdirp-promise? Since, AFAICT, the new version already returns a promise, right?

@dherges
Copy link
Contributor Author

dherges commented Feb 9, 2020

Sounds good to me!

@petebacondarwin
Copy link
Member

I just got hit by this when updating AIO 🙀
So I better get on and fix it!

petebacondarwin added a commit that referenced this issue Feb 12, 2020
The `mkdirp-promise` dependency had its own dependency of
`"mkdirp": "*"`. But recently `mkdirp` did a new breaking change
release that is no longer compatible with `mkdirp-promise`.
This was causing the `writeFile()` service never to resolve its own
promise, which resulted in files not getting written to the disk.

This commit removes the dependency on `mkdirp-promise`
since `mkdirp` now supports returning a promise directly.

Fixes #287
petebacondarwin added a commit that referenced this issue Feb 12, 2020
The `mkdirp-promise` dependency had its own dependency of
`"mkdirp": "*"`. But recently `mkdirp` did a new breaking change
release that is no longer compatible with `mkdirp-promise`.
This was causing the `writeFile()` service never to resolve its own
promise, which resulted in files not getting written to the disk.

This commit removes the dependency on `mkdirp-promise`
since `mkdirp` now supports returning a promise directly.

Fixes #287
@petebacondarwin
Copy link
Member

Thanks for raising this @dherges - it must have been quite a pain to debug what was going on!

@dherges
Copy link
Contributor Author

dherges commented Feb 24, 2020

you're welcome!

I had this thought to modify the dgeni code to work with different versions of the dependencies, and that would have required several "if/else" checks...

Just updating the dependencies (mkdirp >= 1.0.0) sounds very good to me!

Splaktar added a commit to angular/material that referenced this issue Mar 14, 2020
- pin `dgeni-packages` to `0.27.5` as `0.28.0` includes a breaking change
 - https://github.com/angular/dgeni-packages/blob/master/CHANGELOG.md#0280-12-july-2019
- pin `mkdirp` to `0.5.1` to be compatible with `dgeni-packages@0.27.x`
 - angular/dgeni-packages#287
Splaktar added a commit to angular/material that referenced this issue Mar 14, 2020
- pin `dgeni-packages` to `0.27.5` as `0.28.0` includes a breaking change
  - https://github.com/angular/dgeni-packages/blob/master/CHANGELOG.md#0280-12-july-2019
- pin `mkdirp` to `0.5.1` to be compatible with `dgeni-packages@0.27.x`
  - angular/dgeni-packages#287
Splaktar added a commit to angular/material that referenced this issue Mar 17, 2020
- pin `dgeni-packages` to `0.27.5` as `0.28.0` includes a breaking change
  - https://github.com/angular/dgeni-packages/blob/master/CHANGELOG.md#0280-12-july-2019
- pin `mkdirp` to `0.5.1` to be compatible with `dgeni-packages@0.27.x`
  - angular/dgeni-packages#287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants