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

feat: add Serialize type #2578

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

feat: add Serialize type #2578

wants to merge 2 commits into from

Conversation

nullndr
Copy link
Contributor

@nullndr nullndr commented May 23, 2024

Changelog

Currently BullMQ is lying with the data type workers operate with.

Consider the following code:

type Payload = {
  date: Date;
};

new Worker<Payload>("queue", async ({ data: { date }}) => { 
  // Typescript is ok with this, but Javascript will happily throw a TypeError
  console.log(date.toISOString());
});

This is because the type we use does not correctly reppresent the data object we think we operate with. You can imagine data declared as data = JSON.parse(JSON.stringify(...));.

This is the same issue the remix.run team had to deal with a while ago (Indeed the type I added is actually just a little refactor of it).

This PR adds the Serialize type used by the Worker class.

The Serialize type serializes the DataType we pass to the Worker. With this the previous example correctly raise a compilation error in ts.

Notes

Please note that this is likely to be a breaking change, since some users may just pass around the data object without any checking.

@manast
Copy link
Contributor

manast commented May 23, 2024

This is an interesting PR. I wonder if you have a link with more information regarding the type Serialize, as I guess this has been discussed in some other forums, it is not completely trivial to understand how it works. Also as you mentioned, it will imply a breaking change so we need to plan this change carefully.

@nullndr
Copy link
Contributor Author

nullndr commented May 23, 2024

@manast the primary ref for this is the type Jsonify from type-fest.

Other packages have also been proposed on SO, but these are primary focusing on the serialization of the value, not the type.

@manast
Copy link
Contributor

manast commented May 23, 2024

how can we know that the implementation of Serialize is correct?

@nullndr
Copy link
Contributor Author

nullndr commented May 23, 2024

The remix team uses a bunch of utilities that will raise a typescript error if something is wrong. I can implement those as well.

Check them here.

@manast
Copy link
Contributor

manast commented May 23, 2024

Is the remix license incompatible with BullMQ? I wondering in case we could just use that code as is?

@nullndr
Copy link
Contributor Author

nullndr commented May 23, 2024

The licensing is not really my best field, but remix.run is under the MIT, so it should be fully compatible with BullMQ.

in case we could just use that code as is

I truly understand your worries but the version in this PR is a little better to the remix.run, primary on the Prettify type. (The remix.run one is declared erroneusly)

@nullndr
Copy link
Contributor Author

nullndr commented May 23, 2024

This latest commit fixes a previous typo for the serialization of Promise, sorry!

@manast
Copy link
Contributor

manast commented May 24, 2024

I think that before we can merge this we are going to need a pretty good test coverage to avoid breaking existing codebases if for instance typescript would complain on legitimate types and so on.

@nullndr
Copy link
Contributor Author

nullndr commented May 24, 2024

I completely agree, let me know how I can help

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