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

Feat: Support Convenient Directory Imports for Developers Using ESM #783

Open
ITenthusiasm opened this issue Jan 15, 2024 · 7 comments
Open

Comments

@ITenthusiasm
Copy link
Contributor

ITenthusiasm commented Jan 15, 2024

The Problem

Currently, supertokens-node does not (conveniently) support ESM imports because it is written in CJS. As #459 points out, this means that imports such as

import Session from "supertokens-node/recipe/session";

must be changed to

import Session from "supertokens-node/recipe/session/index.js";

This is not the end of the world since it's still possible to import from supertokens-node. But it is a minor inconvenience. And for developers not familiar with some of these import-related nuances in Node.js, it will cause confusion and result in "pseudo bug reports" like #459. It seems that slowly but surely, more web framework tools are migrating to ESM. So this will probably be more highly desired in the future. (I'm not sure when.)

The Solution

Node.js makes it clear how it expects imports to work. And it also provides guidance for supporting CJS and ESM in the same project.

Ultimately, the solution comes down to properly exposing exports for supertokens-node. There are a few ways to go about this. One way is to simply create ESM wrappers for all of the CJS modules (https://nodejs.org/api/packages.html#dual-commonjses-module-packages). These wrappers can probably be generated automatically at build time. In my Form Observer project, I do the opposite (creating CJS from ESM, amidst a few other things).

In the meantime, where appropriate, it might be worth pointing out in the docs that supertokens-node is written in CJS, and to set proper expectations regarding imports for ESM projects. (A link to the Import Specifiers section of Node.js is probably sufficient for informing developers on how to import the resources in supertokens-node.)

Urgency

Not necessarily urgent since ESM projects can still import from supertokens-node and there are still tools that need to migrate to ESM. But the urgency may spontaneously spike at some point in the future. So it's worth at least being aware of this. (Again, no idea when that would be. So I wouldn't freak out about it right now unless it seems easy to do.)

Additional Notes

1) I'd potentially be willing to look into this in the future, but I'm not able to at the moment. 😅 I'd have to get more familiar with the directory structure and build system of supertokens-node first. And I have to attend to some other things before I can devote time to that.

2) I noticed PR #565. It might be worth noting that a new build system isn't necessary to accomplish this task. However, if a newer build system would help make supertokens-node more maintainable, it's certainly worth investigating. (I didn't scrutinize the PR too much. So I can't confirm or deny whether or not the suggested changes there are profitable... though they do seem to shave off quite a few lines of code.)

3) These changes can be done incrementally -- whether a new build system is introduced or not. ESM apps can already import from supertokens-node. So if it's easier to migrate the project file-by-file or folder-by-folder (for instance, by introducing ESM wrappers), that's also an option. This might minimize the concern for unexpected breaking changes. However, I'd still recommend automating the generation of the ESM files in the end. (Or, if the project migrates to ESM, then automating the generation of CJS files.)

@anuragmerndev
Copy link
Contributor

Hello @ITenthusiasm I would like to work on this task. Can I do it directly or I need to have that assigned ? for other tasks as well

@rishabhpoddar
Copy link
Member

Hey @anuragmerndev you can start work on this without being assigned - thanks! I would however recommend that the PRs are made in a way that are small, well tested and well documented - otherwise it will be difficult for us to review.

@anuragmerndev
Copy link
Contributor

Alright thank you, Sure the I'll take care of the PR

@anuragmerndev
Copy link
Contributor

@rishabhpoddar the approach that @ITenthusiasm has shared of adding the exports in package.json explicitly seems good for now so should I implement it in that way or a new build system to handle this issue ?

@ITenthusiasm
Copy link
Contributor Author

@anuragmerndev I can't speak for RP, but in my opinion adding a new build system introduces additional complexity. This increases the risk for unexpected bugs, and it might cause your PR to be reviewed/approved more slowly.

My recommendation would be to simply make sure that the package.json exports work as expected since that will be needed regardless of whether or not we use a new build system. If a new build system makes the exports effort significantly easier without introducing significant complexity, then it might be worth including in your work. Otherwise, it's probably easier to stick to the exports.

The build system can always be revisited later if that's of interest.

@anuragmerndev
Copy link
Contributor

anuragmerndev commented Apr 26, 2024

@ITenthusiasm yeah that's what i thought as well we can do exports in package.json and I can go folder by folder to make smaller PR that can be easily reviewed

@anuragmerndev
Copy link
Contributor

@rishabhpoddar @ITenthusiasm I have added fixed this issue raised a PR for the same, Kindly review #824

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

No branches or pull requests

3 participants