Change CommonJS imports to ES6 imports/exports

[rickstaa]

Is your feature request related to a problem? Please describe.

Currently, we use the (old) CommonJS modules way of importing and exporting modules:

https://github.com/anuraghazra/github-readme-stats/blob/dd5a41d696c3f6a6ad84253317d1b70bd0e1a66f/src/cards/repo-card.js#L11-L14

https://github.com/anuraghazra/github-readme-stats/blob/dd5a41d696c3f6a6ad84253317d1b70bd0e1a66f/src/cards/repo-card.js#L188

Describe the solution you'd like

I think changing to the ES6 way cleans up the code base and makes the package more future-proof.

[rsk2]

Hi @rickstaa, I would like to contribute. So if I understand correctly, the above mentioned imports should change to use ES6 syntax as below:

import I18n from "../common/I18n";

Also the export would as below:

export const renderRepoCard = (repo, options = {}) => {
....
}

[rickstaa]

@rsk2 First of all, great that you want to become a contributor to the GRS repository. Welcome to the team🚀.

You are correct. That is what I had in mind when writing this backlog item. Although we do not enforce it yet, I think it is best to use airbnb's styleguide. What do you think?

Shall I assign you to this backlog item? Don't feel pressured while implementing this item. Although I think it will greatly improve the codebase, it is not an urgent issue.

[rsk2]

@rickstaa Yes, thanks

[rickstaa]

Great. 👍🏻 See CONTRIBUTING.md for tips on how to work on the GRS project. A guide on using the Vscode debugger with Vercel can be found here.

[rsk2]

@rickstaa, do you want me to change the imports/exports in all files as part of this issue or are there any specific files I need to exclude?

[rickstaa]

@rsk2 I planned to change all files since es6 imports/exports are now recommended. So unless the change breaks something, which I don't suspect, no files need to be excluded.

[rsk2]

@rickstaa I am making changes to all the *.test.js files since they also contain CommonJS modules way of importing and exporting. Can you please confirm how to run the tests?

[rickstaa]

Amazing! The most basic way would be to execute npm run tests

https://github.com/anuraghazra/github-readme-stats/blob/579cb7d175d09c4d2c24742a331134e4ed8f027b/package.json#L7

I, however, always use the following VsCode extensions to run and debug them:

• https://marketplace.visualstudio.com/items?itemName=hbenl.vscode-test-explorer
• https://marketplace.visualstudio.com/items?itemName=kavod-io.vscode-jest-test-adapter
• https://marketplace.visualstudio.com/items?itemName=ms-vscode.test-adapter-converter

[rsk2]

@rickstaa I have made the changes to use ES6 import/export. And I have tested it by running on my local as per the instructions in CONTRIBUTING.md and it works. Screenshots below:

However, the tests are failing. I get errors for all the import statements: Cannot use import statement outside a module. Examples below:

Please advice on how to fix this

[rickstaa]

@rsk2 Ah, I think that is because jest does not yet natively support es modules, and we have the test folder outside of the main module (see https://jestjs.io/docs/ecmascript-modules).

However, I have not encountered this issue since I mostly work with typescript, where ts-jest takes care of this problem. Maybe, let's keep the test on the CommonJs syntax until it is added to jest, or the GRS repo is switched to typescript? 🤔

[rsk2]

@rickstaa even if we do keep the test files on the CommonJS syntax, the test fails when it executes the ES import statements in the file being tested. Here is a screenshot of the error I get when running utils.test.js:

Please advise.

[rickstaa]

@rsk2 Ah, sorry, I overlooked that problem. 😅 Like I said in typescript all the heavy lifting to make this possible is done by ts-jest. The only solution would be the one explained in https://stackoverflow.com/a/69059786/8135687 and the documentation https://jestjs.io/docs/ecmascript-modules. I'm fine with implementing that solution but maybe first, let's check how @anuraghazra thinks about supporting an experimental jest feature. 🤔

[Rudxain]

@rsk2 can you please open a PR? I want to help, even if it's just a little bit of help (and other users may want to contribute to the PR too)

[rickstaa]

@Rudxain Thanks for wanting to contribute. I think @rsk2 is already done implementing this feature but runs into https://github.com/anuraghazra/github-readme-stats/issues/1955#issuecomment-1231933253. Do you know a solution other than https://github.com/anuraghazra/github-readme-stats/issues/1955#issuecomment-1232100954 that might work? 🤔 @rsk2, maybe you can update https://github.com/rsk2/github-readme-stats/tree/feature/GRS-1955-change-commonjs-imports so we can see what has already been done 🤓. No pressure, I'm very thankful someone is working on this 🙏🏻.

[Rudxain]

Do you know a solution other than #1955 (comment) that might work? 🤔

Currently, no 😅, but that error message looks very familiar to me. It's the same error msg I would get if I tried to run (in a browser) a script that imports a module. I don't have enough experience with Jest, but I guess you're right about Jest not supporting ESM (yet)

[rsk2]

@rickstaa @Rudxain I have changed the CommonJS import/export to ES6 imports/exports and updated https://github.com/rsk2/github-readme-stats/tree/feature/GRS-1955-change-commonjs-imports. The tests are still failing as discussed. Should I create a pull request?

[Rudxain]

Should I create a pull request?

From my subjective opinion, I think it's a good idea, but only as "draft". When tests go fine, then it should be switched to a "standard PR"

[rsk2]

@Rudxain OK, I have created a draft PR.

[Rudxain]

@rsk2 Nice!

@rickstaa I remembered something. Is NodeJS configured correctly? The problem might not only be Jest, but also Node. It has a flag that must be activated for it to support ESM, it's not "ON/true" by default. I remember trying to run a script using ESM format and got the same error message I was talking about

[anuraghazra]

I'm fine with implementing that solution but maybe first, let's check how @anuraghazra thinks about supporting an experimental jest feature. 🤔

As long as tests doesn't fail or doesn't produce false positives I'm fine with it.

[rsk2]

Hey @rickstaa, I tried to implement the solution that is provided in https://stackoverflow.com/a/69059786/8135687, however I am still getting some errors.

[rickstaa]

@rsk2 Thanks for letting me know. I just tried fixing these errors myself (see https://github.com/anuraghazra/github-readme-stats/tree/change_to_es_imports), but I keep getting the following errors:

SyntaxError: The requested module '@testing-library/dom' does not provide an export named 'queryByTestId'

This is strange since these queries are exported when I inspect the @test-library/dom package. Maybe this is because of some configuration value. @anuraghazra did you encounter this problem before? I have only used jest with typescript, and it is working fine there. Maybe a configuration or dependency problem?

[rickstaa]

@rsk2 I managed to do this in an example repository https://github.com/rickstaa/es6-jest-example. So now the only thing left is where https://github.com/anuraghazra/github-readme-stats/tree/change_to_es_imports differs from this example repo.

[rickstaa]

@rsk2 Okay, I fixed those errors and found the right combination of dependencies (see https://github.com/anuraghazra/github-readme-stats/tree/change_to_es_imports). Two small jest errors still have to be fixed and the code has to be cleaned up.

[rsk2]

@rsk2 Thanks for letting me know. I just tried fixing these errors myself (see https://github.com/anuraghazra/github-readme-stats/tree/change_to_es_imports), but I keep getting the following errors:

SyntaxError: The requested module '@testing-library/dom' does not provide an export named 'queryByTestId'

This is strange since these queries are exported when I inspect the @test-library/dom package. Maybe this is because of some configuration value. @anuraghazra did you encounter this problem before? I have only used jest with typescript, and it is working fine there. Maybe a configuration or dependency problem?

I was getting the same

[rickstaa]

@rsk2 I managed to fix all ES6-related errors. 🎉 I will fix 2 unrelated issues and clean up the code a bit. I will create a PR on your branch when I'm done 👍🏻.

[rickstaa]

@rsk2 I just created https://github.com/rsk2/github-readme-stats/pull/1 to fix the errors you're having. Feel free to merge it and make a PR to https://github.com/anuraghazra/github-readme-stats.

[rsk2]

Thank you @rickstaa, I have merged it. My original PR has been updated with your commit and I have also resolved merge conflicts.

[rickstaa]

@rsk2 Great! Feel free to request a review when you ready.

[rsk2]

@rickstaa , yes I think I have already done that. Please let me know if that's not the case