Add JavaScript implementation of SwedishPersonalIdentityNumber about activelogin.identity HOT 16 OPEN

We should probably look really into pros and cons of all the different ways of exporting the library so that it makes most possible sense in a JS/TS context.

For example, look at just a couple of different ways:

// a very csharpy way
export class SwedishPersonalIdentityNumber {
    public static parse(s: string): SwedishPersonalIdentityNumber { ... }
    
    constructor(
        public year: number,
        public month: number,
        public day: number,
        public birthNumber: number,
        public checksum: number) {
    }
}

// an alternative way

// active-login.models.ts
export interface SwedishPersonalIdentityNumber {
    year: number;
    month: number;
    day: number;
    birthNumber: number;
    checksum: number;
}

// active-login.utils.ts
export function parse(s: string): SwedishPersonalIdentityNumber { ... }
export function parseInSpecificYear(s: string, year: number): SwedishPersonalIdentityNumber { ... }

What I'm trying to say is that I don't have an answer of the best approach here and now, but we should look into it before just following out c# instincts 😊

There are things to consider like tree-shakablility, compatability with different versions of JS/TS with or without boundler/transpilers, common practices, etc.

Super nice that we are bringing this feature into the spotlight again! 😁

from activelogin.identity.

Do you think it would be worth considering creating a completely new github repository for it? Or should we keep both implementations (C#/TS) in the same?

What would you propose? Maybe separating them as it is two completely different build and release pipelines and they could (and should) probably be versioned separately.

from activelogin.identity.

When I use the npm package with my frontend only (for which I have a backend in for example .net) then I just want to validate input from a user before I pass it along to my backend. Then I only expect to use a subset of the full npm-package, I am not interested in instantiating an IdentityNumber type in my frontend application, I am happy to just pass along the string (which I now know is valid) to my backend.

I see your point, but I also see scenarios when you wan't to do the same thing as in the backend. A SPA could potentially render the PIN in different formats and then we need to instantiate it. The overhead in having all the features in the backend would be minimal after minification and GZipping, so I don't see that as a problem. The upside of having a uniform API across backend and frontend would weight that up I'd say. If we modularize it correct internally tree-shaking would be able to step in and maybe cut away some things.

from activelogin.identity.

Any thoughts on what functionality should be included? Just some functions for creating/validating/parsing?

from activelogin.identity.

Any thoughts on what functionality should be included? Just some functions for creating/validating/parsing?

I'd vote for an implementation with the same features as the .NET one. That would enable to enhance the client side UI with validation and normalization of the PIN. I would like to reuse the concept and naming from the .NET implementation, but of course follow JavaScript conventions where those apply.

We have been discussing transpiling the F# implementation into JavaScript, but after some thought I think we should implement it in JavaScript (TypeScript) from scratch to really embrace the things specific for this language.

from activelogin.identity.

I agree with implementing in typescript.

Do you think it would be worth considering creating a completely new github repository for it? Or should we keep both implementations (C#/TS) in the same?

from activelogin.identity.

I am for splitting this to another repo, for the very reasons you state.

Should there be 2 npm packages even? One for server side use including the IdentityNumber type and one with just the validation-function?

When I am building a frontend application for use with for example a .net backend I just want to do some simple validation of user input in my javascript frontend. I would like to depend on an as small as possible package for that.

Or is that a moot point with bundling and tree-shaking? Maybe the size of my final js-bundle wouldn’t matter even if our npm-package includes stuff that wouldn’t be of use to me?

from activelogin.identity.

I'm for splitting as well. The reasons you point out is the reason I brought up the question to start with.

And also that the audience could possibly be very different. So contributing would be easier and too if the javascript/typescript one is its own isolated repo.

I'm having trouble finding a clear distinction between what be considered client and sever side functionality in this module. Could you elaborate on your thoughts @viktorvan?

from activelogin.identity.

Maybe I am out of my element here :)

But what I mean is that someone using this npm package for a node application would expect it to have the same functionality that we have in the nuget package. I.e. I expect there to be a type, I expect there to be a factory-method to create that type and I expect there to be a method to parse a string to get an instance of the type.

When I use the npm package with my frontend only (for which I have a backend in for example .net) then I just want to validate input from a user before I pass it along to my backend. Then I only expect to use a subset of the full npm-package, I am not interested in instantiating an IdentityNumber type in my frontend application, I am happy to just pass along the string (which I now know is valid) to my backend.

Does that make sense? @fredrik-lundin

from activelogin.identity.

And there is already a JS implementation that only does validation, so if someone really only wants a super lightweight implementation, that could be an alternative:
https://github.com/personnummer/js

from activelogin.identity.

Completely agree with @PeterOrneholm on this. From my experience it's very common to use the same npm packages for both server and client side.

We'll obviously gonna have to make sure it's written in a tree-shakable way. That shouldn't be an issue for this kind of library.

I say 1 npm package, 1 repo :)

from activelogin.identity.

I've started looking at this, here is a proposal for the JavaScript/TypeScript public interface:

export class SwedishPersonalIdentityNumber {
    public static parse(s: string): SwedishPersonalIdentityNumber { ... }

    public static parseInSpecificYear(s: string, year: number): SwedishPersonalIdentityNumber { ... }

    constructor(
            public year: number,
            public month: number,
            public day: number,
            public birthNumber: number,
            public checksum: number) {
    }

    public to10DigitStringInSpecificYear(year: number): string { ... }

    public to10DigitString(): string { ... }

    public toString(): string { ... }
}

Compared to the .NET version it lacks .TryParse*() as this does not seem to be the convention in JavaScript, for example JavaSript does not have out parameters.

A few questions that arises:

1. If we create this in a separate repo, what should it be called? I've looked at a few and following the NPM convention it seems like activelogin-identity could be a good fit for both NPM name and repo. What do you think?
2. If you call the constructor, .parse() or .parseInSpecificYear() with invalid data, what should be returned or should we throw an error? parseInt() returns NaN, but that we I don't think we should use, as we are not handling numbers. Date.parse() also returns NaN if invalid, while new Date() returns false. JSON.parse() on the other hand throws an error. I vote for throwing an error for all three methods.

from activelogin.identity.

1. Should it not be activelogin-identity-swedish for the npm package and activelogin-identity for the repo?

2. I vote for throwing an error

And an additional question:
What's the convention in javascript when in comes to using a constructor vs a factory method?

from activelogin.identity.

1. Should it not be activelogin-identity-swedish for the npm package and activelogin-identity for the repo?

Correct, thanks!

What's the convention in javascript when in comes to using a constructor vs a factory method?

Looking at Date it has constructor for composing from the individual numbers, and a parse method for parsing. But in addition has an overload on the constructor for parsing as well...
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date

from activelogin.identity.

2. while new Date() returns false

This was wrong, I did not tets it right. Turns out it throws an error:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Invalid_date

from activelogin.identity.

What I'm trying to say is that I don't have an answer of the best approach here and now, but we should look into it before just following out c# instincts 😊

This is exactly the kind of feedback I'd love to have, thank you! I'd say we should stick with the one that fits closest to how the JavaScript community wants it, that's why we do it native JS/TS :)

Do you have any good docs on these conventions? I've mostly been looking at Date, which I'd say follows the pattern I've described above. But I do recognize the pattern you describe.

How does the second one work if you just add it as a script on a page, I guess we wouldn't expose parse on window but rather wrapped in some scope?

from activelogin.identity.