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

Remove rudimentary node bindings and undocumented %node extension #6285

Merged
merged 3 commits into from
Jun 3, 2023

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Jun 3, 2023

ReScript currently ships with very rudimentary node bindings.

Removing these as it does not make sense to try to maintain them as part of the compiler. A community package like https://github.com/TheSpyder/rescript-nodejs is a better place to maintain such bindings.

Also removing the undocumented %node extension.

@cknitt cknitt requested a review from cristianoc June 3, 2023 16:23
@cknitt cknitt marked this pull request as ready for review June 3, 2023 17:42
@cknitt cknitt merged commit 53e24ed into rescript-lang:master Jun 3, 2023
@cknitt cknitt deleted the remove-node branch June 3, 2023 17:43
@TheSpyder
Copy link
Contributor

TheSpyder commented Jun 29, 2023

This broke rescript-nodejs because the Node module and types were removed. My project relies on them as a shared type, similar to how rescript-webapi relies on the Dom module.

Is this intentional? If I have to ship types in my project that might make it incompatible with previous ReScript releases.

@cknitt
Copy link
Member Author

cknitt commented Jun 29, 2023

@TheSpyder Sorry! I had meant to contact you about this anyway but then forgot. 😞

I think the Node module in the compiler was very incomplete and is not something that should be maintained as part of the compiler. IMHO rescript-nodejs should be the source for the Node.js types instead, just like rescript-react-native is the source for the React Native types.

If you could release a new version of rescript-nodejs for ReScript 11 where Buffer.t is abstract instead of an alias for Node.buffer, then users of your library would not be affected if they were already using the type from your library before, and otherwise they would have to adapt their code accordingly (Node.buffer -> NodeJs.Buffer.t).

What do you think? Am I missing any other issues?

/cc @cristianoc

@TheSpyder
Copy link
Contributor

True, I guess it wouldn't break projects using rescript-nodejs by itself. And I have considered doing this in rescript-webapi so I can take advantage of the juicy records-as-objects types and make object properties easier to access.

The concern I had, and still have with DOM types, is interop with other libraries that use the shared type. I suppose if Node is deleted from core I can just make a new major version and swap to my own types 🤔 anyone sticking on an older version of ReScript will just have to use an older version of the library.

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.

3 participants