Skip to content

Fix clean method to return Promise always - #3501

Merged
taion merged 2 commits into
react-bootstrap:masterfrom
Rokt33r:fix/build-script
Mar 6, 2019
Merged

Fix clean method to return Promise always#3501
taion merged 2 commits into
react-bootstrap:masterfrom
Rokt33r:fix/build-script

Conversation

@Rokt33r

@Rokt33r Rokt33r commented Feb 25, 2019

Copy link
Copy Markdown
Contributor
~/Code/react-bootstrap/tools/build.js:79
  .then(() =>
   ^

TypeError: clean(...).then is not a function
    at Object.<anonymous> (~/react-bootstrap/tools/build.js:79:4)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:741:12)
    at startup (internal/bootstrap/node.js:285:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:739:3)

At 79 lines in build.js, it using .then method. But the current clean method returns false | Promise. So, if there is no lib directory, it throws the above error because clean() will returns false rather than Promise<false>.

clean()
  .then(() =>
    ...
  )

Comment thread tools/build.js Outdated
const esRoot = path.join(libRoot, 'es');

const clean = () => fse.existsSync(libRoot) && fse.remove(libRoot);
const clean = async () => fse.existsSync(libRoot) && fse.remove(libRoot);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems sort of weird... can we just use removeSync here and remove the .then below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also possible. 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you want. Then I'll fix it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just do removeSync here and remove the .then below

@taion
taion requested a review from jquense March 6, 2019 18:22
@taion
taion merged commit 6f03223 into react-bootstrap:master Mar 6, 2019
@Rokt33r
Rokt33r deleted the fix/build-script branch March 8, 2019 04:14
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 this pull request may close these issues.

3 participants