-
Notifications
You must be signed in to change notification settings - Fork 82
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
Comments
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 |
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. |
Alright thank you, Sure the I'll take care of the PR |
@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 ? |
@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 The build system can always be revisited later if that's of interest. |
@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 |
@rishabhpoddar @ITenthusiasm I have added fixed this issue raised a PR for the same, Kindly review #824 |
The Problem
Currently,
supertokens-node
does not (conveniently) support ESMimport
s because it is written in CJS. As #459 points out, this means that imports such asmust be changed to
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
forsupertokens-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 theImport Specifiers
section of Node.js is probably sufficient for informing developers on how to import the resources insupertokens-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.)The text was updated successfully, but these errors were encountered: