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

GRS as an npm module! #2473

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

GRS as an npm module! #2473

wants to merge 2 commits into from

Conversation

Zo-Bro-23
Copy link
Collaborator

The path towards #2179; releases GRS as an NPM module.

Note: Currently the NPM module only supports ESM, meaning that CJS users who try to do require() will get an error. We should consider using babel to automatically convert ESM to CJS before releasing to NPM.

Things are currently working on my end, and I've published a test package @zo-bro-23/github-readme-stats-test. This is only a draft though, and let's work on more features (such as automatic releases with GH Actions) and fix issues with the current code.

@vercel
Copy link

vercel bot commented Jan 26, 2023

@Zo-Bro-23 is attempting to deploy a commit to the github readme stats Team on Vercel.

A member of the Team first needs to authorize it.

@@ -1,5 +1,4 @@
#!/usr/bin/env sh
. "$(dirname -- "$0")/_/husky.sh"

npm test
npx lint-staged
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kept showing errors when I tried commiting. @rickstaa any idea why that is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what causes the error. You need to update the tests a bit to let them work with your pull request. You can debug these tests with the https://github.com/jest-community/vscode-jest plugin. Sadly, I don't have time to look at it for the next three months 😅.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should stay since we want the tests to run when people try to commit. 🤔

if (!cache_seconds && (isBothOver1K || isBothUnder1)) {
cacheSeconds = CONSTANTS.FOUR_HOURS;
}

res.setHeader(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't seem to be very effective as the default cache is anyways FOUR_HOURS. Let me know if I should add it back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to be very effective as the default cache is anyways FOUR_HOURS. Let me know if I should add it back.

Think this should be EIGHT_HOURS now that we increased the default to FOUR_HOURS.

"name": "github-readme-stats",
"version": "1.0.0",
"name": "@zo-bro-23/github-readme-stats-test",
"version": "1.0.5",
"description": "Dynamically generate stats for your GitHub readme",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only temporary. I will change it to the actual package name before merging.

"technote-space",
"sw-yx",
"blacklistedUser", // For testing purposes
];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a problem? Makes testing easier.

Copy link
Collaborator

@rickstaa rickstaa Jan 28, 2023

Choose a reason for hiding this comment

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

It should not be a problem.


if (blacklist.includes(username)) {
return renderError("Something went wrong");
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we should change this to a throw instead of a return. That way, users can choose to show the error card or not. Currently, if I use the NPM package, this is what my code will look like:

import express from "express";
import { generateStatsCard } from "@zo-bro-23/github-readme-stats-test";

const app = express();

app.get("/", (req, res) => {
  res.setHeader("Content-Type", "image/svg+xml");
  generateStatsCard({ username: "Zo-Bro-23" }).then((card) => {
    res.send(card);
  });
});

app.listen(8000);

The .then() gets fired for errors too, and even if I added a .catch(), it would never get fired. Separating errors and successful responses might be the best way to move forward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes sense when people use this as an NPM package. 👍🏻 I think @anuraghazra added it this way to fix some renovate problems. 🤔

export * from "./cards/index.js";
export { getStyles, getAnimations } from "./getStyles.js";
export * from "./generators/index.js";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any reason why you're exporting these? These should be internal functions, and not exported. Keeping these as is bloats up the NPM package when I use import *, since everything from the CONSTANTS to the utils functions gets loaded in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wanted to allow other developers to use these functions when they implement new cards. You are right however if the npm package is setup the right way people can just use the Card class. 👍🏻

@ofek
Copy link

ofek commented May 6, 2023

Is this almost ready?

@neverbot
Copy link

neverbot commented Sep 6, 2023

Hi!

I think this is blocking #2537 which is blocking #2603, which is a top bug and I think is making the stats unusable for a lot of people (in my case, 9 out of 10 times they do not appear).

Is there something we can do?

@rickstaa
Copy link
Collaborator

rickstaa commented Sep 12, 2023

The path towards #2179; releases GRS as an NPM module.

Note: Currently the NPM module only supports ESM, meaning that CJS users who try to do require() will get an error. We should consider using babel to automatically convert ESM to CJS before releasing to NPM.

Things are currently working on my end, and I've published a test package @zo-bro-23/github-readme-stats-test. This is only a draft though, and let's work on more features (such as automatic releases with GH Actions) and fix issues with the current code.

Pending Tasks:

  • Decide whether to support CJS modules: In my view, supporting this legacy feature may not be necessary, considering there hasn't been an npm package utilizing it in the past. We could opt to support ESM modules exclusively. I'd like to hear the opinions of @Zo-Bro-23, @anuraghazra, and @qwerty541 on this matter.
  • Pull Request Review: One of us, including other collaborators, should review this pull request. Additional eyes on the code can help expedite the merge process. 👍🏻
  • Create GRS npm Organization and Package Release: It's essential that @anuraghazra establishes the GRS npm organization and proceeds with the package release.

@Zo-Bro-23, please feel free to add any other tasks that may have been overlooked.

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.

None yet

4 participants