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

Support @new @variadic #5364

Merged
merged 2 commits into from
Jun 10, 2022
Merged

Conversation

cannorin
Copy link
Contributor

Closes #5363.

This PR allows @new and @variadic to be used simultaneously.

'use strict';
class Foo {
    constructor(...names) {
        this.names = names;
    }
}
let foo = new Foo("bar", "baz");
console.log(foo.names);
type foo
@get external names: foo => array<string> = "names"
@new @variadic external makeFoo : array<string> => foo = "Foo"
let foo = makeFoo(["bar", "baz"])
Js.log(foo->names)

@cannorin cannorin mentioned this pull request Mar 18, 2022
6 tasks
@bobzhang
Copy link
Member

bobzhang commented May 5, 2022

Would @raw cover your use case?
We would like to have diffs small for easy review

@cannorin
Copy link
Contributor Author

cannorin commented May 5, 2022

Would @raw cover your use case?

%raw can't easily workaround this when the class is imported.

Also, it's kind of unnatural for me (and perhaps for many other users) that @new @variadic does not work. Docs also doesn't mention they don't work when used together.

We would like to have diffs small for easy review

"small diffs" means no changes to jscomp/main/builtin_cm*_datasets.ml and lib/**/*, right?

@bobzhang
Copy link
Member

bobzhang commented May 5, 2022

"small diffs" means no changes to jscomp/main/builtin_cm*_datasets.ml and lib/**/*, right?

Yes, ideally no white space changes or in a separate commit.

@cannorin
Copy link
Contributor Author

cannorin commented May 5, 2022

@bobzhang done 👍

@cannorin
Copy link
Contributor Author

cannorin commented Jun 8, 2022

No pressure, but it would be great if this gets included in v10.0.

I continue to be available for resolving any reviews and change requests in the foreseeable future.

@cristianoc
Copy link
Collaborator

@cannorin happy to take a look, but unsure what aspect should be checked.
Anything that one should focus on? That existing features are not affected? That all cases are covered? Code style? Others?

@cannorin
Copy link
Contributor Author

cannorin commented Jun 9, 2022

@cristianoc thank you!

Since the PR is about modifying @new so that it allows @splice to be used together, the main concerns would be:

  • the existing @new codes would not break
  • the coding style

@cristianoc
Copy link
Collaborator

OK I'll have a go. Might ask some basic question about the codebase.
Meanwhile: CI looks red.

jscomp/core/lam_compile_external_call.ml Outdated Show resolved Hide resolved
jscomp/core/lam_compile_external_call.ml Outdated Show resolved Hide resolved
jscomp/frontend/external_ffi_types.pp.ml Show resolved Hide resolved
jscomp/runtime/caml_splice_call.ml Show resolved Hide resolved
jscomp/runtime/caml_splice_call.ml Outdated Show resolved Hide resolved
jscomp/test/splice_test.ml Show resolved Hide resolved
jscomp/core/lam_compile_external_call.ml Show resolved Hide resolved
jscomp/core/lam_compile_external_call.ml Outdated Show resolved Hide resolved
jscomp/core/lam_compile_external_call.ml Show resolved Hide resolved
@cristianoc
Copy link
Collaborator

Also, the changelog should be updated.

@cannorin
Copy link
Contributor Author

cannorin commented Jun 9, 2022

Thanks! I will work on changelog and CI tomorrow.

Signed-off-by: Yuta Sato <[email protected]>
@cannorin
Copy link
Contributor Author

applied the reviews 🚀

@cannorin
Copy link
Contributor Author

cannorin commented Jun 10, 2022

Error: Conflicting attributes: Attribute found that conflicts with %@new

hmm, it seems that the modified compiler is not used during CI run?

I didn't stage the following files. Do I need to check in these files to make CI use the modified compiler?

        modified:   jscomp/main/builtin_cmi_datasets.ml
        modified:   jscomp/main/builtin_cmj_datasets.ml
        modified:   jscomp/test/splice_test.js
        modified:   lib/4.06.1/unstable/js_compiler.ml
        modified:   lib/4.06.1/unstable/js_playground_compiler.ml
        modified:   lib/4.06.1/whole_compiler.ml
        modified:   lib/es6/caml_splice_call.js
        modified:   lib/js/caml_splice_call.js

@cannorin
Copy link
Contributor Author

I try to stage everything in a separate commit (so that it can be easily reverted) and see what happens

@cannorin
Copy link
Contributor Author

macos-arm didn't run but everything else is now green 🚀

@cristianoc
Copy link
Collaborator

Sometimes macOS-arm needs a bit of encouragement.
Good to go. Ready to merge if there's nothing outstanding.

@cristianoc cristianoc merged commit 7b206bf into rescript-lang:master Jun 10, 2022
@cannorin cannorin deleted the variadic-new branch June 10, 2022 14:42
jihchi pushed a commit to jihchi/rescript-compiler that referenced this pull request Jun 12, 2022
* Support @new @variadic

Signed-off-by: Yuta Sato <[email protected]>

* [variadic-new] stage everything

Signed-off-by: Yuta Sato <[email protected]>

Co-authored-by: Yuta Sato <[email protected]>
mununki pushed a commit to mununki/rescript-compiler that referenced this pull request Jul 15, 2022
* Support @new @variadic

Signed-off-by: Yuta Sato <[email protected]>

* [variadic-new] stage everything

Signed-off-by: Yuta Sato <[email protected]>

Co-authored-by: Yuta Sato <[email protected]>
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.

@new @variadic is not supported?
4 participants