869210 - Clean up make build output

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P1)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

make build output currently is very scary to any newcomer and super noisy for us.

http://pastebin.mozilla.org/2380325

Comment 1

[Staś Małolepszy :stas]

Attachment: Silence "Failed to find module: amdefine"

The patch removes the "Failed to find module: amdefine" errors by defining a dummy amdefine module and then removing it from the build.  It's a work-around until a fix lands in DryIce:  https://github.com/mozilla/dryice/issues/17

The output now looks like this:

Creating dist/l20n.js
- l20n/html.js has define(...) with non-array parameter. Ignoring requirement.
- l20n.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/context.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/events.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/parser.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/compiler.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/promise.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/retranslation.js has define(...) with non-array parameter. Ignoring requirement.
- Found several matches for l20n/platform/globals.js (ignoring 2nd)
  - /home/stas/moz/l20n/js/lib/client/l20n/platform/globals.js
  - /home/stas/moz/l20n/js/lib/l20n/platform/globals.js
- l20n/platform/globals.js has define(...) with non-array parameter. Ignoring requirement.
- Found several matches for l20n/platform/io.js (ignoring 2nd)
  - /home/stas/moz/l20n/js/lib/client/l20n/platform/io.js
  - /home/stas/moz/l20n/js/lib/l20n/platform/io.js
- l20n/platform/io.js has define(...) with non-array parameter. Ignoring requirement.
- l20n/platform/intl.js has define(...) with non-array parameter. Ignoring requirement.

I like having "Found several matches for l20n/platform/globals.js " in there, but I'd still like to get rid of the "Ignoring requirement." errors.  It looks like this was intended by the author of DryIce to be helpful, but it clearly doesn't work well with the Simple CommonJS Wrapper, which DryIce supports well.  I might try to submit a pull request to make this output optional if I find a moment, but it's not critical.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'd like to remove the Ignoring requirement and for the found several matches - why do we need it? Why can't Makefile point to the right one?

As of now it seems like the AMD is confused about the multiple matches which is a non-optimal state of things

Comment 3

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #2)
> I'd like to remove the Ignoring requirement and for the found several
> matches - why do we need it? Why can't Makefile point to the right one?

I'm using DryIce's createCommonJS helper which follows require() calls and names anonymous modules automatically based on the file name.  This way I don't have to explicitly list all files in the Makefile.

Admittedly, it doesn't cost us anything to do so.  I'll experiment tomorrow.

Comment 4

[Staś Małolepszy :stas]

Attachment #746339 [details] [diff] landed in https://github.com/l20n/l20n.js/commit/33e0a304a976dda9c32e5411c268843343402c36

I'm leaving the bug open to try to fix things mentioned in comment 2.

I submitted https://github.com/jrburke/amdefine/pull/15 which is related.

Comment 5

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #4)

> I'm leaving the bug open to try to fix things mentioned in comment 2.

I need further changes in amdefine for this to work: https://github.com/jrburke/amdefine/pull/16

Comment 6

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #5)
> I need further changes in amdefine for this to work:
> https://github.com/jrburke/amdefine/pull/16

The pull request was rejected, so I took another approach and made changes to dryice instead:

https://github.com/mozilla/dryice/pull/33

Comment 7

[Staś Małolepszy :stas]

Joe merged my changes into his branch:

https://github.com/joewalker/dryice/commit/a0cdb595ec17fe1938ac5d338a602414d0de396c

I landed a change to package.json to use hsi branch for now.  When he upstreams these changes to mozilla/dryice, I'll follow up here.

I also made a small change to how out built files look like:  I removed the deps array from define calls, because we don't need them anyways.

Landed: https://github.com/l20n/l20n.js/commit/bc328b15a07e809a81f4c7bea1749f3a7161f0ca

Here's what the current output of make build looks like:

node build/Makefile.js html

Creating dist/html/l20n.js
- Found several matches for l20n/platform/globals.js (ignoring 2nd)
  - /home/stas/moz/l20n/js/lib/client/l20n/platform/globals.js
  - /home/stas/moz/l20n/js/lib/l20n/platform/globals.js
- Found several matches for l20n/platform/io.js (ignoring 2nd)
  - /home/stas/moz/l20n/js/lib/client/l20n/platform/io.js
  - /home/stas/moz/l20n/js/lib/l20n/platform/io.js

Creating dist/html/l20n.min.js