doc/melange.rst - make the alias attachment behavior of melange.emit clearer

[haochenx]

• rendered: https://dune--7924.org.readthedocs.build/en/7924/melange.html
• related: melange - add melange.emit targets to @all alias as well by default #7926

Before #7926, targets declared by melange.emit stanza is not attached to the dune's default @all stanza, which can be confusing. #7926 changes that and this PR aims to make the document clearer.

original (and out-of-date) pr description

Following the introduction of the current melange document page I was confused by why running `dune build` would not produce the javascript outputs and only realize that the `melange.emit` stanza by default attach the targets to the `melange` alias instead of the default alias after a very careful examination of the documentation.

It would be nice to make the default alias attachment clear in the introduction section to avoid potential confusions of new adopters.

(initial) diff:

-After running ``dune build output/hello.js``, Dune produces the following
-file structure:
+After running ``dune build output/hello.js`` or ``dune build @melange``
+(by default, all ``melange.emit`` stanzas will attach their targets to
+the  ``melange`` alias),
+Dune produces the following file structure:

that is, the following text being inserted:
"or dune build @melange (by default, all melange.emit stanzas will attach their targets to the melange alias)"

[haochenx]

fyi: @anmonteiro

[haochenx]

Sorry that my previous force push (to fix a layout problem) wasn't containing the intended fix and I just force pushed again.

[jchavarri]

Looks good. I wonder if it's better to remove the explicit build of output/hello.js altogether, as it breaks as soon as there are two modules, and it's not the recommended way to build Melange projects in any case. @anmonteiro wdyt?

[jchavarri]

This behavior is explained in the alias field of the melange.emit stanza docs, a few paragraphs below. I am not sure it's worth adding a reference?

[haochenx]

When I was following the introduction section and got hello.js in the minimal example built, I started to hacking the minimal example around and was met with surprises when dune build (without specifying the melange alias) wasn't generating the javascript files.

I personally wasted some decent time trying to debug it was the case and only realized such behavior after very carefully reading the document again.

Yes the behavior is already documented, but I think it's probably not be only me getting the impression that the targets added by melange.emit stanza will be attached to the default alias (as for most other dune stanzas) after just following the current introduction.

[jchavarri]

I was not aware that all other targets (including jsoo ones, it seems) are added to the default alias. I am onboard with adding melange.emit targets to the default alias, but I'm not sure if this addition was avoided for some reason that I'm not aware of. @rgrinberg @anmonteiro do you know?

(Probably something that should be discussed in a separate issue)

[rgrinberg]

I think adding it to the default alias (@all in this case) is the correct thing to do.

[haochenx]

I wonder if it's better to remove the explicit build of output/hello.js altogether

I have no preference. @anmonteiro should have a better say.

[anmonteiro]

@haochenx thanks for starting the discussion on this. I agree the language is confusing, but I'm concerned we might have added more confusion in this PR by explaining too much.

Given the other comment, I wonder whether this would be an appropriate course of action:

1. Open a new PR adding melange.emit to the default alias
2. In this PR, mention "after dune build, we get the following project structure" (and we can explain aliases later on)
3. in the aliases section, add a note about melange.emit JS rules being attached to the default alias @all since Dune 2.0

@haochenx would you like to tackle 1. ? I believe it'd be a matter of adding more rules to the implicit default alias here:

dune/src/dune_rules/melange/melange_rules.ml

Lines 326 to 335 in 0591719

	and+ () =
	match mel.alias with
	| None ->
	let alias =
	Alias.make Melange_stanzas.Emit.implicit_alias ~dir:target_dir
	in
	Rules.Produce.Alias.add_deps alias deps
	| Some alias_name ->
	let alias = Alias.make alias_name ~dir:target_dir in
	Rules.Produce.Alias.add_deps alias deps

You can get the default alias with: (Build_config.get ()).implicit_default_alias dir

[Alizter]

If you want to attach melange.emit to default, we can do it in time for 3.8.2. Just open a PR or issue and add it to #7907.

[rgrinberg]

You can get the default alias with: (Build_config.get ()).implicit_default_alias dir

You could, but that would be a mistake. The default alias can be customized by the user so we don't want to attach to it directly and prevent the user from changing it. By "default", it's set to @all. So we should attach it to @all.

If you want to attach melange.emit to default, we can do it in time for 3.8.2. Just open a PR or issue and add it to #7907.

We don't add features to point releases.

[anmonteiro]

By "default", it's set to @all. So we should attach it to @all.

Oh right, we should be using Alias.Name.all.

[haochenx]

I am also in favor of adding the targets to @all (the default dune alias) by default and it seems to be the consensus here.

I’m happy to send a PR making such a change and update this PR to reflect the point (2) and (3) by @anmonteiro.

My concern is that this will be a breaking change to an already shipped feature. I’m not sure whether silently changing the behavior is a good thing to do. Maybe we would like to bump the plugin version? (i.e. only change the default alias if the user specify (using melange 0.2) instead of (using melange 0.1) in dune-project)

But if we agree that given the experimental status of the current melange support it’s acceptable to have such “silent” breaking changes. It’s definitely easier to implement and maintain this way.

Also, regarding the structure of the PRs, maybe it would make sense to close this PR and address the document change in the PR that change the default alias attachment. I think this will be clearer retrospectively. edit: rereading the comments and I’m realizing that we’d have the benefit to be able to deal with the implementation and documentation separately by separating the PRs, and that’s probably more favorable.

[anmonteiro]

But if we agree that given the experimental status of the current melange support it’s acceptable to have such “silent” breaking changes. It’s definitely easier to implement and maintain this way.

My opinion is that we don't care too much in this phase and we can ship this change. If others feel differently, I'm happy to defer to stronger opinions.

Also, regarding the structure of the PRs, maybe it would make sense to close this PR and address the document change in the PR that change the default alias attachment. I think this will be clearer retrospectively.

Happy to keep the alias change and the documentation in a separate PR.

[rgrinberg]

It's acceptable. The only potential for breakage is for those already using this experimental feature.

[haochenx]

Now #7296 is merged and I have updated the patch to document accordingly.
(following (2) and (3) of this suggestion: #7924 (comment))

PTAL @anmonteiro @jchavarri @rgrinberg @christinerose

[haochenx]

It seems that the windows CI job is stalling:

https://github.com/ocaml/dune/actions/runs/5241726516/jobs/9464209550?pr=7924

It might be a good idea to restart it (I doesn't seem to have the permission to do so)

[jchavarri]

Should we remove the reference to build an individual js file and replace it with dune build? Something like was done in the first commit, e.g.:

- After running ``dune build output/hello.js``, Dune produces the following
- file structure:
+ After running ``dune build @melange`` or just ``dune build``, Dune
+ produces the following file structure:

One of my conclusions from this PR is that including instructions to build individual js files can be confusing and error prone (the approach breaks as soon as more js files get added).

[jchavarri]

Thank you! Just one suggestion, see previous comment.

[haochenx]

Should we remove the reference to build an individual js file and replace it with dune build? Something like was done in the first commit,

Ah! Indeed. I made that change but it seems that I somehow accidentally thrown away that change down the way.
Thank you for pointing it out. @jchavarri

I believe it's now reflected in 77d2154 8fc805e

[haochenx]

@jchavarri @anmonteiro Thank you!

[anmonteiro]

Thanks again!

[haochenx]

Thank you all!