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

Designing v4.0.0 #226

Open
alexandercerutti opened this issue Oct 30, 2024 · 5 comments
Open

Designing v4.0.0 #226

alexandercerutti opened this issue Oct 30, 2024 · 5 comments

Comments

@alexandercerutti
Copy link
Owner

alexandercerutti commented Oct 30, 2024

Here we go: this project is ready to see its next iteration designed.

So, I'm going to write down some possible ideas to improve this package. I'd like to receive some feedbacks from you.


Upgrading minimum supported version to Node 20 and distribute only ESM version of this package

Its time to move on. Node 14 has been in out of maintenance for long time now.

Furthermore the ESM support is now very wide and Node 23 will also ship import of ESM modules from CJS modules, unlocking a lot of opportunities (starting from Node 24, which will be LTS).

The idea is to convert the whole package to ESM and then, optionally, distribute the CJS version as well.


Adding support to ESM exports fields

The usage of exports could unlock us a lot of features. The first that comes to my mind is the support to utils to read model and certificates from fs.

In fact, some platforms like Cloudflare workers, do not actually support all the features from Node and not all usages require the usage of fs. So, why should we load code people are not going to use?

My current idea could be something like this.

import { fromFs } from "passkit-generator/node/fs";

const pass = new PKPass(
    fromFs("./path/to/model"), // the risk here is to require setting everything as asynchronous, so might not be worth the pain, unless reading synchronously
    {
        signerCert: fromFs("./path/to/signerCert"),
        ...
    },
    { ... }
);

const pass = await PKPass.from(
    {
        model: fromFs("./path/to/model"),
        certificates: {
            signerCert: fromFs("./path/to/signerCert"),
            ...
        },
    },
    { ... }
);

/**
 * Same risk of asychronicity here
 */
pass.addBuffer("icon.png", fromFs("./path/to/icon.png"));

Move from Buffer to Uint8Array (if possible)

  • This would allow us to have broader support to platforms outside Node. Buffer, today, extends Uint8Array, so they should be the same.

  • Also, addBuffer would become semantically wrong. A better name might be found.

  • This would probably also allow passkit-generator to run, even if in an unsupported way, on a web browser and in a "splitted way", which means that a part of passkit-generator is run on a WebBrowser and part of it (signature with certificates) is run on the server. This is a feature to be planned in case. I might need it for pkvd.app in the future.

  • Update: the change is available in the branch feature/uint8array and brings with it some breaking changes. Applications using Express (or firebase functions or serverless) will have to change how streams and buffer are sent:

For getAsBuffer():

response.status(200).send(Buffer.from(pass.getAsBuffer()));

This because, at the current situation:

  • ExpressJS doesn't support Uint8Array to be sent (I've opened a PR)
  • Same might be valid for serverless

For getAsStream(), streams have been converted to ReadableStream from node:stream/web. So, in an application that uses NodeJS Streams, it is better to do something like:

import { Readable } from "node:stream";

Readable.fromWeb(stream).pipe(response);

Supporting again reading certificates by paths

The support dropped in v3.0.0 because it wasn't easy to recognize if the content was a certificate (string) or a path. However, we could now thing to use either the proposal above fromFs or the URL constructor to represent a local resource that should be retrieved.

This represents a risk of asynchronicity as well, so should be evaluated very correctly.


Splitting the Bundle integration from passkit-generator.

By doing this, we going to create a new package that will serve as a common base for Wallet Orders future package.

This will bring a big refactor to move everything is required (like l10n / .pass) processing to the new package.


Better errors

Errors should get rewritten by using classes that extends the native class Error. Something like this.

From:

export const PREFERRED_STYLE_SCHEMES = {
	UNEXPECTED_PASS_TYPE_SET:
		"Cannot set preferredStyleSchemes on a pass with type different from eventTicket.",
	UNEXPECTED_PASS_TYPE_GET:
		"Cannot get preferredStyleSchemes on a pass with type different from eventTicket.",
	INVALID:
		"Cannot set preferredStyleSchemes because not compliant with Apple specifications - %s",
} as const;

To:

export class UnexpectedPassTypeForProperty extends Error {
    constructor(property: string, expectedType: string) {
        super();
        
        this.name = "UnexpectedPassTypeForProperty";
        this.message = `Cannot set ${property} on a pass with type different from ${expectedType}`;
    }
}

export class ForbiddenRetrieveForProperty extends Error {
    constructor(property: string, expectedType: string) {
        super();
        
        this.name = "ForbiddenRetrieveForProperty";
        this.message = `Cannot get ${property} on a pass with type different from ${expectedType}.`;
    }
}

export class PropertyComplianceViolation extends Error {
    constructor(property: string, error: string) {
        super();
        
        this.name = "PropertyComplianceViolation";
        this.message = `Cannot set ${property} because not compliant with Apple specifications - ${error}`;
    }
}

Furthermore, errors should not be automatic anymore, as they can cause confusion.
This means that getters and setters for properties like transitType, should have prior validation on boardingPass.

This list will be updated as soon as other ideas and comments will pop up.

@MN-987
Copy link

MN-987 commented Dec 2, 2024

what about Implementing a deno package for passkit-generator I tried to use it in supabase but couldn't

@alexandercerutti
Copy link
Owner Author

Never tried Deno. However, I heard that now it should be mostly compatible, on v2, with node modules.

If you can try and let me know what it needs to work, I think we can discuss it. 👍

@MN-987
Copy link

MN-987 commented Dec 5, 2024

It did not work, actually. I even uploaded the whole project to Firebase just to be able to use the PassKit Generator package. I am actually thinking about implementing a PassKit Generator package for Deno if you are down for it.

@alexandercerutti
Copy link
Owner Author

@MN-987 In down for supporting Deno inside here. However, I'm asking you what went wrong, which errors you got.

If you can open a new issue with them and all the details you can, I can try to see where the package is failing (or your code is, I don't know). Otherwise I have no starting point.

Then, depending on the difficulty of the fixes, I'll try to understand if it can be done in a minor or in a major.

@MN-987
Copy link

MN-987 commented Dec 5, 2024

Alright I'll try to list and reproduce the bugs again because the project I was working on was while ago and I'll open issues with them

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

No branches or pull requests

2 participants