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

Typescript Support #27

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dbrenot-pelmorex
Copy link

@dbrenot-pelmorex dbrenot-pelmorex commented Jan 26, 2023

This PR addresses #25 by adding typescript support to the project.
This PR also fixes webpack support mentioned in #15, bringing the number of open issues down to 0 if this is merged!

This shouldn't hinder the developers of the library and has a relatively low learning curve. Some things were changed from functions that return a "module" to classes with a constructor. This is functionally the same pattern and more standard, so use doesn't change much, if at all.

Most function signatures remain unchanged aside from getSott, which before was under the helper object in the loginradius function. Now it is directly exposed from the login radius API instead. This is a very small change in the code of users that use this function(lrv2.helper.getSott becomes lrv2.getSott). Additionally if they are using an IDE, this should show up in the editor and should be fairly obvious how to fix it. The README has been updated to accommodate this small change as well.

This PR adds a build script and prepublish that would run automatically when the library is published to npm, so there is no complicated process to learn or tweak in order to work with this. Just publish as you usually would and the script will handle the rest.

This PR didn't add any types for returns. The return types of the functions isn't immediately clear to me and would take a substantial amount of time. I'm sure another community member would be happy to do this, or someone at the loginradius team.

Lastly, this PR adds support for ESM as well as CJS by building both. This will ensure this library is compatible with both old and new nodejs versions and different environments.

  • Renamed files from .js to .ts
  • Added ESM support. Very important as node considers these the standard format moving forward
  • Cleaned up code comments to follow jsdoc correctly
  • Added types to API parameters to give intellisense when using
  • Added build script that runs on prepublish, generating types, CJS and ESM
  • Added node_modules to gitignore
  • Switched from dynamic require statements to imports, supporting webpack and ESM
  • Removed helper and other objects from config and split them out into separate functions that can be called standalone

Please let me know your thoughts! I put quite a bit of time into this PR since we intend to use it at my workplace and wished to share the code with the community.

@dbrenot-pelmorex dbrenot-pelmorex changed the title Renamed files to .ts and initial project configuration(WIP) Typescript Support(WIP) Jan 26, 2023
@dbrenot-pelmorex dbrenot-pelmorex marked this pull request as draft January 27, 2023 18:31
@dbrenot-pelmorex dbrenot-pelmorex marked this pull request as draft January 27, 2023 18:31
@dbrenot-pelmorex dbrenot-pelmorex changed the title Typescript Support(WIP) Typescript Support Jan 27, 2023
@dbrenot-pelmorex dbrenot-pelmorex marked this pull request as ready for review January 27, 2023 18:39
@indrasen715
Copy link
Collaborator

Hi @dbrenot-pelmprex
Thanks for bringing this to our notice, and we appreciate your valuable feedback on this.

I will review this feedback with the team and keep you posted on the progress.

Regards
Indrasen

@dbrenot-pelmorex
Copy link
Author

Bumping this since it's been a month, any interest in this? We need to keep the pr in a private repo locally until this is merged so I was hoping this could get merged.

@indrasen715
Copy link
Collaborator

Hi @dbrenot-pelmorex

I hope you are doing great!

We would like to inform you that we have already taken this as feedback and have added this to our SDK enhancement list.

As of now, we will suggest using the Typescript changes in your private repo locally and we will keep you updated once our development team has a chance to review this feature request, it may be scheduled (if approved and validated, as it depends on various factors) in our SDK Development roadmap for a future release.

Thanks

@dbrenot-pelmorex dbrenot-pelmorex mentioned this pull request May 25, 2023
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.

2 participants