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

Fix nested object get and set not working in res files #533

Merged
merged 16 commits into from
May 5, 2023
Merged

Conversation

jchavarri
Copy link
Member

@jchavarri jchavarri commented Mar 30, 2023

Fixes #489.

@jchavarri jchavarri marked this pull request as ready for review March 30, 2023 14:58
@jchavarri jchavarri requested a review from anmonteiro March 30, 2023 14:58
@jchavarri
Copy link
Member Author

jchavarri commented Mar 30, 2023

@anmonteiro the fix is ready (relevant commit: ef79a3b), but I think I need some help configuring dune so that rescript-syntax package is added as dep of this test. 😅

* main:
  melange: Add a `melpp` executable that just preprocesses conditionals (#539)
  Promote test files (#537)
  chore: update nix flake (#536)
* main:
  update dune to latest (#540)
@jchavarri
Copy link
Member Author

I think I need some help configuring dune so that rescript-syntax package is added as dep of this test.

I updated with latest main, but still see some error in ci due to rescript-syntax not being available:

- File "test/dune", line 13, characters 11-26:
- 13 |   (package rescript-syntax)))
-                 ^^^^^^^^^^^^^^^
- Error: Package rescript-syntax does not exist

I can reproduce when running dune build -p melange @runtest. I am surprised by this error, shouldn't dune just ignore tests from packages that are uninstalled? Related: ocaml/dune#3650

@anmonteiro
Copy link
Member

-p is only-packages. How about dune build runtest -p melange,rescript-syntax,reactjs-jsx-ppx?

@jchavarri
Copy link
Member Author

Yes, this works, see Makefile:

melange/Makefile

Lines 23 to 24 in f550ed9

test:
opam exec -- dune runtest -p melange,rescript-syntax,reactjs-jsx-ppx

The problem happens when installing melange, because melange.opam has the following on build:

melange/melange.opam

Lines 24 to 31 in f550ed9

"dune"
"build"
"-p"
name
"-j"
jobs
"@install"
"@runtest" {with-test}

@anmonteiro
Copy link
Member

Ah. Feel free to not run the tests then. I remove that line from the opam file when we release to opam too.

@jchavarri
Copy link
Member Author

I disabled rescript-syntax tests in ff9a0c5. But this was quite unfortunate as there seems to be no way to keep checking the test added in this PR in the future.

So I followed a different alternative in e2073f8:

  • add the new rescript-syntax test to melange package
  • remove the --with-test flags when pinning the 3 packages melange, rescript-syntax and reactjs-jsx-ppx in opam-install-test
  • add back make test to ci

That way, we can keep testing everything while working around dune limitation for running tests with -p. Lmk what you think! :)

@jchavarri
Copy link
Member Author

Meh, it worked locally, but in ci everything breaks. Nix build is broken and opam build as well. Will take another stab at this later.

@anmonteiro
Copy link
Member

Happy to fix the nix stuff. the opam build is failing because reason isn't installed

@jchavarri
Copy link
Member Author

@anmonteiro I think I got it to a point where everything works without any workarounds. There were a series of issues. I ended up creating a folder test/rescript-syntax and moving the new test of this package over there, to avoid having to use applies_to, which seems it's a bit buggy and cumbersome to use. I also started using %{bin:xxx} dependencies rather than package ones, which seems to work and doesn't lead into the issue you reported in ocaml/dune#7220.

@jchavarri jchavarri changed the title test: add repro for 489 Fix nested object get and set not working in res files Apr 13, 2023
* main:
  feat(melange): add `--preamble` for e.g. "use client" (#545)
  fix(ci): never re-run the `anonymous` function
  test: interaction between `[@@@bs.config {flags = ...}]` and `--as-ppx`
  melange: delete dead code across the ext library (#552)
  melc: delete Ext_json and related modules, use dune-build-info for (#551)
  melc: remove old namespace system, legacy mode (#550)
  chore: changelog entry for #548
  feat(melange): installable and usable in more OCaml versions (#548)
  fix(melange): improve error message for file-level flags handler (#549)
  melange: allow vendoring without having node.js installed (#547)
  [lite-version] Separate into melange.ppx and melange.ppx-lib (#534)
  chore: remove mel (#546)
  chore: update flakes (#543)
  Add test for doc comment attached to `%%private` (#542)
@jchavarri jchavarri requested a review from anmonteiro May 5, 2023 14:19
* main:
  [@new @varidic]: use `Function.prototype.bind` instead of `new C(...args)` (#558)
  Fix a warning when compiling the C stubs with OCaml 5 (#5887)
  Fix typo in error message (#5823)
  Fix typo
  Modify jscomp/runtime/js.ml too
  Add placeholder types for ES6 collections
  fix Js_math deprecation message
  Turn on optimizations for unicode strings.
  Add new function to Option: orElse (#5400)
@anmonteiro
Copy link
Member

Thank you!

@anmonteiro anmonteiro merged commit dfe9990 into main May 5, 2023
@anmonteiro anmonteiro deleted the gh-489 branch May 5, 2023 20:47
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.

Nested object get and set not working in res files
2 participants