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

Add the ability to remap EsModule imports at link time #2737

Merged
merged 2 commits into from Feb 18, 2024

Conversation

Quafadas
Copy link
Contributor

WIP is a rather kind description. A cry for help, perhaps.

This attempts to follow up on VirtusLab/scala-js-cli#47, and close #2698 - i.e. introduce a using directive and / or command line option, to enable the work done in scala-js-cli.

This introduces a failing test, which I can run with;
./mill -i 'integration.test' 'scala.cli.integration.RunTests213.remap imports'

My current headscratcher, is that I don't seem to be able to modify the behaviour of scala-cli itself though. I've liberally put some printlns everywhere - which aren't printed. So I think I'm suffering from some sort of new contributor syndrome.

Would anyone have a hint, on why the intregration test, could be ignoring my attempts to introspect it?

@Gedochao Gedochao self-requested a review February 15, 2024 10:34
@Gedochao
Copy link
Contributor

One thing this made me realise is that we never bumped scala-js-cli to 1.15.0.1, so I'm doing it in #2738... (I know you're overriding the version in the test, just mentioning, still need to look into your code)

@Gedochao
Copy link
Contributor

My current headscratcher, is that I don't seem to be able to modify the behaviour of scala-cli itself though. I've liberally put some printlns everywhere - which aren't printed. So I think I'm suffering from some sort of new contributor syndrome.

Can you point me at where you're trying to introspect, which prints don't show?

Would anyone have a hint, on why the intregration test, could be ignoring my attempts to introspect it?

a wild guess is that you are printing on stdout, while stdout is being piped in the test...
https://github.com/com-lihaoyi/os-lib/blob/6603eb0e7434c86721806f2c1922452d9d9e49fc/os/src-jvm/ProcessOps.scala#L63-L65
If you want to print the logs when running the integration test (as opposed to when you call a freshly built launcher directly), then you need

os.proc(...).call(cwd = root, stdout = os.Inherit)

as stdout defaults to os.Pipe.
alternatively, you can print on stderr, which defaults to os.Inherit.
No idea if this was the question 😅

@Gedochao
Copy link
Contributor

beyond the above, I'd recommend debugging the launcher directly, instead of printing stuff in the integration tests.

./mill -i integration.test.jvm 'scala.cli.integration.RunTests213.remap imports' --debug
# (...)
# --debug option has been passed. Listening for transport dt_socket at address: 5005

@Quafadas
Copy link
Contributor Author

Would you have a hint on how to fix this?

I think it's somehow checking the documentation...

image

@Gedochao
Copy link
Contributor

Would you have a hint on how to fix this?

Yes.
https://github.com/VirtusLab/scala-cli/blob/main/CONTRIBUTING.md#rules-for-a-well-formed-pr
You need to regenerate the reference docs, among other things.

./mill -i generate-reference-doc.run

Also, you may want to rebase on top of main, rather than cherry picking individual commits.

@Quafadas
Copy link
Contributor Author

Quafadas commented Feb 16, 2024

@Gedochao @armanbilge This actually ... works ... to my satisfaction.

The happy path works well.

I have a question on error messaging, which is that if I (deliberately) break it, for example by entering a bad file path to my importmap, it produces the error message I'd expect, but it's hidden at the top of the error stack, above scala-cli help. My eyeballs are where the green line is, but the message I need, is where the red line is.

Given that I'm naively throwing an exception, could I be engaging with scala-cli's error handling the wrong way?

Screenshot 2024-02-16 at 09 48 30

@Gedochao
Copy link
Contributor

Given that I'm naively throwing an exception, could I be engaging with scala-cli's error handling the wrong way?

@Quafadas Rather than throwing an exception, you need to return a Left[BuildException].
You'll need to define your own exception type extending BuildException (refer to the existing errors that are returned).

@Quafadas
Copy link
Contributor Author

Quafadas commented Feb 16, 2024

  • Tighten error messages
  • Tests for error conditions
  • Tests for happy path
  • Manual testing
  • Squash
  • Docs
  • CI green

@Gedochao
Copy link
Contributor

./mill -i __.fix

you need this to satisfy the checks CI job.

@Gedochao Gedochao changed the title WIP: Add the ability to remap EsModule imports at link time Add the ability to remap EsModule imports at link time Feb 16, 2024
Copy link
Contributor

@Gedochao Gedochao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Gedochao Gedochao merged commit fe7a80f into VirtusLab:main Feb 18, 2024
58 checks passed
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.

Offer a way to rewrite scalaJS imports at linktime
2 participants