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

enh(Add open collective prompt) #755

Merged

Conversation

misterdev
Copy link
Contributor

@misterdev misterdev commented Feb 6, 2019

I'm adding a console message to ask the users for donations, following how it is implemented in opencollective-cli

OSX
screenshot 2019-02-06 at 16 15 01

Windows
postinstall

At the moment this is printed by the postinstall script. It can be improved by printing the banner sometimes during the cli usage as suggested in @evenstensberg example

LANG=C DAY=$(date +"%a")

if [ "$DAY" == "Mon" ];
then
    printDonationHeader

I think we can print it during cli usage with probability 1/50, I will work on this.

Did you add tests for your changes?
No. Should I?

If relevant, did you update the documentation?
It's not relevant

Summary
Adds a "support us on open collective" closing #752
Closes #752

Does this PR introduce a breaking change?
No

@misterdev misterdev changed the title [WIP] feat(add opencollective msg): add script in package.json & nice prompt [WIP] feat(Add open collective prompt) Feb 6, 2019
package.json Outdated Show resolved Hide resolved
bin/opencollective.js Outdated Show resolved Hide resolved
bin/opencollective.js Outdated Show resolved Hide resolved
bin/opencollective.js Outdated Show resolved Hide resolved
bin/opencollective.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Just left few comments and enhancements! Overall looks good to me!
Should we show the banner only once a week though? Better to not be too invasive

@misterdev
Copy link
Contributor Author

Thanks @ematipico for your feedback, I've included those changes.

I've improved the formatting of the output, now it's prettier and well centered:

screenshot 2019-02-06 at 22 53 52

Also, this is showed once a week, I do this by saving the timestamp of the last print in a file called ./.lastocprint, so every time we run webpack-cli it have to read from the file system. Is this a good solution?

I think this is complete, feedback are welcome 👍

@misterdev misterdev changed the title [WIP] feat(Add open collective prompt) feat(Add open collective prompt) Feb 6, 2019
@misterdev misterdev changed the title feat(Add open collective prompt) enh(Add open collective prompt) Feb 6, 2019
@misterdev
Copy link
Contributor Author

@ematipico you can review this if you want

bin/opencollective.js Show resolved Hide resolved
@webpack-bot
Copy link

@misterdev Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evenstensberg Please review the new changes.

@ghost
Copy link

ghost commented Mar 11, 2019

There were the following issues with this Pull Request

  • Commit: f66368f
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@misterdev misterdev force-pushed the feature/opencollective-postinstall branch from ead8caa to dd9d528 Compare March 11, 2019 15:57
@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

bin/opencollective.js Outdated Show resolved Hide resolved
bin/opencollective.js Outdated Show resolved Hide resolved
@misterdev
Copy link
Contributor Author

The prompt shows only on Monday if the last time it has been shown was more than 6 days earlier.
I've used Stats.atime as suggested by @evenstensberg to determine the last time the file (opencollective.js) has been executed. On windows seems that the atime is not updated when opencollective.js is executed, so I've added an instruction that sets it.

bin/cli.js Outdated
@@ -473,6 +473,22 @@ For more information, see https://webpack.js.org/api/cli/.`);
const statsString = stats.toString(outputOptions);
const delimiter = outputOptions.buildDelimiter ? `${outputOptions.buildDelimiter}\n` : "";
if (statsString) stdout.write(`${statsString}\n${delimiter}`);


const now = new Date();
Copy link
Member

Choose a reason for hiding this comment

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

add comment here , maybe abstract to a local function. Needs more comments and explaination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this comment on top:

/**
* Show a hint to donate to our Opencollective
* once a week, only on Monday
*/

bin/cli.js Outdated
const { statSync, utimesSync } = require("fs");
const lastPrint = statSync(openCollectivePath).atime;
const lastPrintTS = new Date(lastPrint).getTime();
if (now.getTime() - lastPrintTS > SIX_DAYS) {
Copy link
Member

Choose a reason for hiding this comment

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

abstract to expression or function that has the naming installedLaterThanSixDays or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've abstracted to a variable:

const timeSinceLastPrint = now.getTime() - lastPrintTS;
if (timeSinceLastPrint > SIX_DAYS) {

Thanks for your comments, let me know if I can make any other improvements 👍

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

lgtm

@evenstensberg evenstensberg merged commit 9597377 into webpack:master Mar 16, 2019
@misterdev misterdev deleted the feature/opencollective-postinstall branch March 30, 2019 00:18
if (timeSinceLastPrint > SIX_DAYS) {
require(openCollectivePath);
// On windows we need to manually update the atime
utimesSync(openCollectivePath, now, now);

Choose a reason for hiding this comment

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

This line breaks the build on Linux when we install Webpack globally :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:( Could you please open an issue with some details about the error?

Choose a reason for hiding this comment

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

=> #870 :-)

sbrunner added a commit to camptocamp/c2cgeoportal that referenced this pull request Apr 29, 2019
sbrunner added a commit to camptocamp/c2cgeoportal that referenced this pull request Apr 29, 2019
sbrunner added a commit to camptocamp/c2cgeoportal that referenced this pull request Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants