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 obj keys mangle #6354

Merged
merged 2 commits into from
Aug 19, 2023
Merged

Conversation

DZakh
Copy link
Contributor

@DZakh DZakh commented Aug 17, 2023

I've finally found the source of the behavior. Although, I wasn't able to backtrace the reasoning behind it. The code was written 7 years ago by @Hongbo-bb and probably was needed to properly handle @obj or something like this.
But now, I think it only causes wtf moments when the compiled code is not what you expect.

Solves:

Related to:

It's a big breaking change, but according to tests it doesn't break anything.

@cristianoc @cknitt @zth @mununki What do you think?

@cristianoc
Copy link
Collaborator

Removing it seems the best option for sure.
Though one needs to assess the consequences with a few people trying on their projects.
@cknitt @zth @mununki do you have any code at hand that could be affected?

Also worth checking on the doc website whether there's any mention of this trick.

@cknitt
Copy link
Member

cknitt commented Aug 17, 2023

@DZakh Great find! 👍

I tested against our current/biggest project (that's already on v11/uncurried) and found no issues with this change.

@mununki
Copy link
Member

mununki commented Aug 18, 2023

@DZakh Looking great.

I've tested it in my project and there are no issues, and it doesn't affect the functionality of #5904.

@DZakh
Copy link
Contributor Author

DZakh commented Aug 18, 2023

I didn't expect it, but there's actually a note in the docs https://rescript-lang.org/docs/gentype/latest/usage#renaming-gentypeas-and-object-mangling-convention

@DZakh
Copy link
Contributor Author

DZakh commented Aug 18, 2023

I'll go update changelog 🙏

@cknitt cknitt merged commit 61c5592 into rescript-lang:master Aug 19, 2023
14 checks passed
@cknitt
Copy link
Member

cknitt commented Aug 19, 2023

@DZakh I noticed that the name mangling is still present in the Gentype code:

Apply bucklescript's mangling rules for object field names:

@DZakh
Copy link
Contributor Author

DZakh commented Aug 19, 2023

I think we need to remove it from there as well. I'll be able to tackle it around Monday.

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.

None yet

4 participants