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

Add JavaScript implementation of SwedishPersonalIdentityNumber #14

Open
PeterOrneholm opened this issue Oct 12, 2018 · 16 comments
Open

Comments

@PeterOrneholm
Copy link
Collaborator

At the moment, SwedishPersonalIdentityNumber is implemented in C# targeting .NET Standard, which (kind of) limits it to server side use. Implementing it in JavaScript (using TypeScript) and publish it as an NPM package would enable client side parsing and validation scenarios as well as using it from node.js on the server.

@fredrik-lundin
Copy link
Collaborator

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

@PeterOrneholm
Copy link
Collaborator Author

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.

@fredrik-lundin
Copy link
Collaborator

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?

@PeterOrneholm
Copy link
Collaborator Author

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.

@viktorvan
Copy link
Collaborator

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?

@fredrik-lundin
Copy link
Collaborator

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?

@viktorvan
Copy link
Collaborator

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

@PeterOrneholm
Copy link
Collaborator Author

PeterOrneholm commented Nov 25, 2018

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.

@PeterOrneholm
Copy link
Collaborator Author

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

@fredrik-lundin
Copy link
Collaborator

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 :)

@PeterOrneholm
Copy link
Collaborator Author

PeterOrneholm commented Mar 16, 2019

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.

@viktorvan
Copy link
Collaborator

  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?

@PeterOrneholm
Copy link
Collaborator Author

  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

@PeterOrneholm
Copy link
Collaborator Author

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

@fredrik-lundin
Copy link
Collaborator

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! 😁

@PeterOrneholm
Copy link
Collaborator Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants