feat: supporting esm import

anuragmerndev commented 2 months ago

add the exports in package.json to support esm import for js and ts

Summary of change

This pull request addresses issue #783 by adding subpath patterns exports to the package.json file. These exports support ESM (ECMAScript Modules) imports for both JavaScript and TypeScript files, improving compatibility and ease of use for developers. Now, imports like import Session from "supertokens-node/recipe/session"; can be used without specifying the index.js file explicitly.

Related issues

Test Plan

Update the package.json file with the provided exports configuration.
Ensure the project builds without errors.
Verify that existing imports using ESM syntax work as expected.
Test importing modules in both JavaScript and TypeScript files.
Run unit tests to confirm that the changes do not introduce regressions.
Check for any compatibility issues with existing codebases using the library.

https://github.com/supertokens/supertokens-node/assets/144275260/197d89a4-2817-4a33-9334-85adccfba59f

Documentation changes

No documentation changes are required for this PR.

Checklist for important updates

[ ] Changelog has been updated
[ ] coreDriverInterfaceSupported.json file has been updated (if needed)
- Along with the associated array in lib/ts/version.ts
[ ] frontendDriverInterfaceSupported.json file has been updated (if needed)
[ ] Changes to the version if needed
- In package.json
- In package-lock.json
- In lib/ts/version.ts
[ ] Had run npm run build-pretty
[ ] Had installed and ran the pre-commit hook
[ ] If new thirdparty provider is added,
- [ ] update switch statement in recipe/thirdparty/providers/configUtils.ts file, createProvider function.
- [ ] add an icon on the user management dashboard.
[ ] Issue this PR against the latest non released version branch.
- To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
- If no such branch exists, then create one from the latest released branch.
[ ] If have added a new web framework, update the add-ts-no-check.js file to include that
[ ] If added a new recipe / api interface, then make sure that the implementation of it uses NON arrow functions only (like someFunc: function () {..}).
[ ] If added a new recipe, then make sure to expose it inside the recipe folder present in the root of this repo. We also need to expose its types.

Remaining TODOs for this PR

No additional tasks are required for this PR.

ellipsis-dev[bot] commented 2 months ago

Your free trial has expired. To continue using Ellipsis, sign up at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at help@ellipsis.dev

netlify[bot] commented 2 months ago

Deploy Preview for astounding-pegasus-21c111 canceled.

Name	Link
Latest commit	f3e92fb08b62adb7ee0a39c2dd676c2676604260
Latest deploy log	https://app.netlify.com/sites/astounding-pegasus-21c111/deploys/66300a4391332d0008152fac

netlify[bot] commented 2 months ago

Deploy Preview for precious-marshmallow-968a81 canceled.

Name	Link
Latest commit	f3e92fb08b62adb7ee0a39c2dd676c2676604260
Latest deploy log	https://app.netlify.com/sites/precious-marshmallow-968a81/deploys/66300a4373b9540008771d2e

ITenthusiasm commented 2 months ago

From the video, I can see that the build works; but I don't see the examples of ESM-based/CJS-based/TypeScript Node applications working properly. How did those go?

ITenthusiasm commented 2 months ago

Also just giving a reminder to target the proper version branch, as mentioned in the PR checklist.

anuragmerndev commented 2 months ago

From the video, I can see that the build works; but I don't see the examples of ESM-based/CJS-based/TypeScript Node applications working properly. How did those go?

I have updated the video last video didn't capture the other two windows attaching here as well

https://github.com/supertokens/supertokens-node/assets/144275260/900d6be1-a269-4da5-949c-ff134016bfab

anuragmerndev commented 2 months ago

Also just giving a reminder to target the proper version branch, as mentioned in the PR checklist.

Okay I have updated to the latest unreleased branch

ITenthusiasm commented 2 months ago

Thanks for the update! As a heads up, I probably won't be able to look at this until the weekend. There's also rishabhpoddar when he has more availability, though. I'm mainly here to help with making sure that the import/export configuration looks normal.

An optional recommendation: Your commit history might be clearer if you rebase your current changes onto the latest version of the target branch instead of merging that branch's features into your own.

ITenthusiasm commented 2 months ago

So far the new video looks promising though!

anuragmerndev commented 2 months ago

Thanks for the update! As a heads up, I probably won't be able to look at this until the weekend. There's also rishabhpoddar when he has more availability, though. I'm mainly here to help with making sure that the import/export configuration looks normal.

An optional recommendation: Your commit history might be clearer if you rebase your current changes onto the latest version of the target branch instead of merging that branch's features into your own.

Sure I'll do the rebase

ITenthusiasm commented 2 months ago

Hey friend, sorry I wasn't able to get to this. The weekend actually turned out much busier than I expected. But it looks like things should calm down after today so hopefully I'll be able to review soon.

(I know the code change is small. But I want to be able to test the changes as well to double-verify that no unsuspecting breaking changes would sneak in. I'm still a little surprised that we wouldn't need separate files for handling CJS/ESM exports.)

anuragmerndev commented 2 months ago

Sure you can test the same and do let me know if you have any questions on the same

Talking about the require and import in the nodejs documentation for conditional exports it's provided that we can separate the cjs and mjs if we are having two files but we can also give default key which will work for both the scenarios here's the link to documentation

ITenthusiasm commented 1 month ago

@anuragmerndev Can you link this to #459 as well to help with cleaning up the issues?

Separately, I'm probably going to try to pull this branch locally to test (just to doubly make sure). My first suspicion as to why this would work is that ESM is able to import from CJS (but not the other way around). If that's the case then yeah, this should be smooth sailing.

anuragmerndev commented 1 month ago

LGTM (Sorry this took so long to review. Been really busy.)

@rishabhpoddar

I only tested importing from supertokens-node/recipe/emailpassword. (@anuragmerndev's example uses significantly more imports.) But for that import, I can confirm that requireing worked just fine, and importing worked for type=module node apps. (importing does not work if the semantic exports property is excluded from the package.json file in supertokens-node).

Some Brief Background

CJS applications cannot import from ESM applications. But ESM applications can import from both CJS and ESM applications. The supertokens-node package does not identify as an ESM package (because type="module" is not in the package.json file); so it is a CJS package. Therefore, when an ESM application tries to import from supertokens-node, it sees it as a CJS package, and the import works fine (because ESM can handle those). Of course, regular CJS applications have no problem importing from supertokens-node currently.

Additional Notes

If there are any other paths that people should be allowed to import/require from, they need to be added to exports in package.json. Otherwise, consumers won't be allowed to access those paths. The exports property in package.json is effectively a "whitelist". If it's sufficient to only allow imports from supertokens-node/recipe/**/*, then I guess this PR is good to go. But if other paths need to be exposed, it would be helpful to know those.

Not sure what you wanted to do about tests. There's probably a way to write those? The test would basically involve successfully importing/requireing without errors -- if you wanted to automate that.

If you guys added documentation on how to write imports for applications using ESM, then that documentation will need to be re-updated when this gets merged. (Otherwise, don't worry about it.)

@anuragmerndev

Thanks for looking into this. (And sorry again for the delay.) I'm sure this change will make a lot of people happy. Could you share a code snippet of the test file that you used in the video? That way, if anyone else wants to test locally, they'll have the ability to do so with ease.

@ITenthusiasm Thank you for the update on the issue, I have created a repository with a README outlining the test procedure for this fix. supertokens-node-import-test

And thank you @ITenthusiasm for sharing your time on checking this PR, means a lot.

@rishabhpoddar

Regarding the exports, as @ITenthusiasm mentioned, only the files within the 'recipe' directory are currently accessible due to the whitelist configuration. If there are any additional paths that should be exposed, please let me know. During my review of the documentation for supertokens-node usage, I only encountered imports related to recipes. If there are other imports we need to include, I'll be happy to update the PR accordingly

rishabhpoddar commented 1 month ago

Thanks @anuragmerndev and @ITenthusiasm for the work so far, really appreciate it. Someone from our team will have a look at this too soon

anuragmerndev commented 1 month ago

Hi, First of all, sorry for the late review. We've been swamped releasing 18.0 and other projects.

As for the PR, I have some concerns:

Please update the PR (and the target branch) to the latest

Some entry points are missing (see my comment), so ideally, we'd like to figure out a way to "test" if this happens again. This is not necessarily a blocker for this PR, but any ideas are welcome.

If we can test that the defined exports match some files, that'd also be awesome.

We need to allow importing from the build output directly, as we've seen some use cases that require that. I know it's less than ideal, but we want to enable pretty extreme customizations. I was thinking of adding something like the following to the exports obj, but it seems relatively awkward, so any ideas are welcome:
        "./lib/*.js": {
            "types": "./lib/*.d.ts",
            "default": "./lib/*.js"
        },
        "./lib/*": {
            "types": "./lib/*.d.ts",
            "default": "./lib/*.js"
        }
Can we also support existing apps that import us using a ".js" module specifier? Otherwise, this PR will be a breaking change.

Hello @porcellus

No worries about the late reviews, In fact, thank you for checking the PR also here's the updates on your concerns

I have updated the PR target branch to 18.0.
I'll add the missing entry points in the final commit I am working on.
I can provide the repository which I shared before in the comments for testing.
For the build exports, I am researching to find a better solution; otherwise, we can proceed with the approach you provided.
We will need to add something like the following to support existing apps using the ".js" module specifier, so this PR won't be a breaking change:

  "./recipe/*": {
        "types": "./recipe/*/index.d.ts",
        "default": "./recipe/*/index.js"
    },
    // for index.js support
    "./recipe/*/index.js": {
        "types": "./recipe/*/index.d.ts",
        "default": "./recipe/*/index.js"
    }

Also I wanted to confirm that do we need to provide this .js support for all the exports you mentioned in the comments ?

porcellus commented 1 month ago

I can provide the repository which I shared before in the comments for testing.

I meant some way of doing this in an automated test, especially testing that we didn't miss anything.

For the build exports, I am researching to find a better solution; otherwise, we can proceed with the approach you provided.

That'd be awesome.

We will need to add something like the following to support existing apps using the ".js" module specifier...

Sounds good

Also I wanted to confirm that do we need to provide this .js support for all the exports you mentioned in the comments?

Yeah, all entrypoints should work either way, otherwise we might break some apps.

anuragmerndev commented 1 month ago

Hello @porcellus,

I have pushed the updated code with support for rest of modules and the .js modules for existing apps along with build folder.

About the test thing I am still researching how can we automate tests for this specific PR, haven't found any till now.

Kindly review the same and let me know for any updates.

porcellus commented 1 month ago

I've reviewed this and it looks good. I'll merge this into an intermediary branch, because I need to add some further changes (I've figured out a way to automatically test it) and add a changelog entry/increase the version number.

anuragmerndev commented 1 month ago

I've reviewed this and it looks good. I'll merge this into an intermediary branch, because I need to add some further changes (I've figured out a way to automatically test it) and add a changelog entry/increase the version number.

Alright thank you,

Also can you please share the logic for automated tests

porcellus commented 1 month ago

You can check out the test here. It's far from pretty, but it already showed some errors we had.

Also, if you'd like, please feel free to add yourself as a contributor in the supertokens-core repo. (similar to this PR) Thank you for contributing to SuperTokens.

anuragmerndev commented 1 month ago

You can check out the test here. It's far from pretty, but it already showed some errors we had.

Also, if you'd like, please feel free to add yourself as a contributor in the supertokens-core repo. (similar to this PR) Thank you for contributing to SuperTokens.

Thank you for letting me contribute will do alot more of it Also here's the PR to the Contributor update Do let me know if need any update

supertokens / supertokens-node