Allow configuring DDC module type

[pulyaevskiy]

Looking at the source code I see that build_web_compilers has hardcoded amd module type when building for web.

Is there a way to override this default behavior?

I'm working on some packages which are designed for NodeJS and this requires common module type.

[pulyaevskiy]

Hm... I just realized that I probably need to write something like build_node_compiler similar to the web-specific package but with different configuration. I'm gonna try this...

[jakemac53]

The main work is changing the bootstrapping logic, which is fairly specific to require.js right now. Would require a bit of reorganization in this package to support it well but it wouldn't be too difficult. Making a node specific package is probably reasonable as well, we could extract the summary and module specific logic out of the build_web_compilers package as well which would help.

[pulyaevskiy]

Thanks @jakemac53 ! I already forked this repo and basing my work on it (it turns out there is a lot of logic around Modules in here).

If you don't mind I'd like keep this issue open to ask questions along the way?

[jakemac53]

Yep, sounds good

[jakemac53]

Also, we can chat on gitter here https://gitter.im/dart-lang/source_gen. We have a lot of follow-up meetings today but will be more available next week.

[pulyaevskiy]

Awesome, no rush at all. Here is a couple of questions I have so far.

For DDC modules to work with Node I'm thinking (after reading a lot) I need to put everything generated by DDC in a node_modules/packages somewhere. The question is are there any conventions around this?

I see build_runner creates .dart_tool/build/generated/{package} folders. Should I just move everything in .dart_tool/build/generated/* to build/node_modules/packages? This is probably not gonna work with incremental builds right?

Is there a way to customize output folders somehow?

Second question: I have a my_package/web/main.dart, which is also inside generated folder.
Ideally I'd like folder structure to look like this:

.dart_tool/
  - generated/
    - node_modules/
      - packages/
        - my_package/
    - web/
      - main.dart.js

Is it possible? Any advise would help a lot!

(also, I wanted to rename web/main.dart to node/main.dart but build_runner didn't pick up this folder for some reason).

[jakemac53]

For DDC modules to work with Node I'm thinking (after reading a lot) I need to put everything generated by DDC in a node_modules/packages somewhere. The question is are there any conventions around this?

This is a very interesting question - I think creating directories under node_modules conceptually maps to a packages folder, but I don't know if it is really intended to be mucked with manually. If you are using npm I don't know what the consequences might be.

I see build_runner creates .dart_tool/build/generated/{package} folders. Should I just move everything in .dart_tool/build/generated/* to build/node_modules/packages?

There is the --output <dir> flag which will create a merged output directory with a packages folder and whatever top level folders you have in your application (web, test, etc).

This should also essentially answer your 2nd question, about moving the web directory to the top level (I think the output dir matches what you want there).

(also, I wanted to rename web/main.dart to node/main.dart but build_runner didn't pick up this folder for some reason)

We have a hardcoded whitelist of common top level directories that are treated as inputs by default. To include other directories you need to create a build.yaml which looks something like this:

targets:
  $default:
    sources:
      - "lib/**"
      - "node/**"

[jakemac53]

It also looks like node supports a package.json file, as well as require.paths global which might be useful to avoid creating an actual node_modules directory.

[pulyaevskiy]

as well as require.paths global

Looks like it was removed from the API a while ago. Though it'd be great if it was there.
I will most likely have to override built-in require function just because DDC generates require calls to a some sort of idiomatic package name which doesn't match with actual file name of the module.

Something like this:

  // somewhere in main.dart.bootstrap.js
  var Module = require('module');
  var originalRequire = Module.prototype.require;

  Module.prototype.require = function () {
    var id = arguments['0'];
    if (id.startsWith('packages/') {
      // assume this is a DDC module, pseudo-code
      var path = modulePaths[id];
      arguments['0'] = path;
      return originalRequire.apply(this, arguments);
    }
    return originalRequire.apply(this, arguments);
  };

but I don't know if it is really intended to be mucked with manually. If you are using npm I don't know what the consequences might be.

Ideally I'd need to use npm to install JS packages next to the generated ones.
With --output=build I get a much better folder structure. Would it be considered ok to have a builder which generates build/packages.json (for npm package dependencies) and runs npm install so that those modules are installed in build/node_modules?

We have a hardcoded whitelist of common top level directories that are treated as inputs by default.

It didn't seem to work for me from the first try.
I have a fork of build_web_compilers, so I modified build.yaml in there as you suggested, might still be doing something wrong.

Thanks a lot for your help! I already got some positive results executing a basic DDC-compiled app in Node with require calls working.

[natebosch]

If you want to modify the files that you can load from your package you'll need to add a build.yaml in that package. Changing the build.yaml in build_web_compilers won't have an impact on the whitelist.

[jakemac53]

Would it be considered ok to have a builder which generates build/packages.json (for npm package dependencies) and runs npm install so that those modules are installed in build/node_modules?

I think generating a packages.json file is reasonable, but I would run npm install as a separate post-build step.

[matanlurey]

What is the actionable item for build? @jakemac53 @natebosch

[natebosch]

Two possibilities for what we could do in build:

• Pull out the module stuff from build_web_compilers into a separate package that we can reuse for this node stuff.
• Make the node module patterns an option to the compilers here.

If we can see a prototype of this running that would help us make a decision.

[matanlurey]

Sounds good. I'll label this as BWC for now.

[pulyaevskiy]

I should have a prototype in a couple of days.
Thank you again @jakemac53 and @natebosch for your help!

[pulyaevskiy]

Small update: I managed to get DDC working with my playground project with only these changes:
master...pulyaevskiy:node-support
(It's very experimental and I'm still learning.)

Basic test with dart2js was successful without any changes (except adding node_preamble to dart2js output).

I will be looking at getting npm dependencies and generating packages.json next.

But it looks very promising so far. Will have some more updates soon.

[pulyaevskiy]

I have a demo app here:
https://github.com/pulyaevskiy/dartconf2018
There are some instructions in the readme.

I pushed more changes to node-support branch in build_web_compilers:
master...pulyaevskiy:node-support

It's a bit more cleaner now, but I didn't do any large refactorings.
I had to add an extra nodePreamble option for dart2js. A better way would probably be to have moduleType = amd | common option. If user wants common this also means node preamble is needed for dart2js.

Also, the trick to override target default sources to allow node/** seems to be only working for DDC, couldn't get it to work with dart2js.

[pulyaevskiy]

Duplicating my questions from Gitter channel here:

---

@jakemac53 @natebosch hi guys! Just checking in about this issue. I pushed a small update to DDC part last night. Also had some success updating firebase_admin interop library with all tests passing in both DDC and dart2js (in this branch https://github.com/pulyaevskiy/firebase-admin-interop/tree/dart20).

I'd like to move forward on this. I thought I could build another package on top of build_web_compilers, but all Module and ScratchSpace APIs are hidden so I can't really reuse it.

I see three options at this point:

1. Expose Module/ScratchSpace APIs in build_web_compilers so that a separate package can be created for Node
2. Add ddcModuleType option to build_web_compilers with all the changes I have in my fork (cleaned up and refactored of course)
3. Fork build_web_compilers, modify and publish as a different package.

Option 3 is least favorite. Option 1 is probably least risky, the new package's functionality may eventually be merged back to build_web_compilers if we see a need for it.

What do you guys think?

[jakemac53]

We talked about this a bit more, and I think where we want to go is most similar to your option 1. Essentially, we can separate out the build_web_compilers package into three different packages, build_modules, build_analyzer_summaries, and build_web_compilers. Then you could take your current logic and create a build_node_compilers package, which depends on build_modules and build_analyzer_summaries.

The main downside is we would end up with some duplicate code between build_node_compilers and build_web_compilers, but the upside would be you can expose your js modules with a different file extension (.node.ddc.js?), and the two packages can share the module/summary inputs, and be used side by side in the same package if desired.

Also users would not be required to do any explicit configuration, they just depend on the package they want.

Thoughts?

[pulyaevskiy]

This sounds good to me!

Looking at the list of builders:

Builder moduleBuilder(_) => const ModuleBuilder();
Builder unlinkedSummaryBuilder(_) => const UnlinkedSummaryBuilder();
Builder linkedSummaryBuilder(_) => const LinkedSummaryBuilder();
Builder devCompilerBuilder(_) => const DevCompilerBuilder();
Builder webEntrypointBuilder(BuilderOptions options) =>
    new WebEntrypointBuilder.fromOptions(options);

The first three would go in build_modules and build_analyzer_summaries and the other two would have to be duplicated in web and node packages?

This is actually already helpful.

Do you guys think you'd have an opportunity to work on extracting those builders any time soon? No problem if not. If it's ok I'd be happy to contribute here.

The way I was thinking to approach this is make a copy of build_web_compilers in this repository and name it build_node_compilers. Then create new build_modules and build_analyzer_summaries and gradually move related logic in there, while refactoring web and node packages. In the end I can just extract the node-specific package out or if you feel it's useful enough for Google can keep it in here (don't have a preference myself).

Let me know what works best for you.

[jakemac53]

The first three would go in build_modules and build_analyzer_summaries and the other two would have to be duplicated in web and node packages?

Yep

Do you guys think you'd have an opportunity to work on extracting those builders any time soon? No problem if not. If it's ok I'd be happy to contribute here.

Either way, we could likely do this within the next few weeks (could be sooner, we are just a bit swamped after dartconf). It shouldn't be much work though. Happy to take pull requests as well if you would like something sooner.

The way I was thinking to approach this is make a copy of build_web_compilers in this repository and name it build_node_compilers. Then create new build_modules and build_analyzer_summaries and gradually move related logic in there, while refactoring web and node packages.

I would prefer to break out the other packages first if you don't mind - it limits the number of breaking releases we need to do for packages.

We could do this bottom up so first build_modules and then build_analyzer_summaries.

In the end I can just extract the node-specific package out or if you feel it's useful enough for Google can keep it in here (don't have a preference myself).

If you are willing to own it, my feeling is its good for the ecosystem to have more externally owned/maintained packages. Plus I think you know more about actually running in node than I do at least haha.

[matanlurey]

Is there a strong rationale to creating too many sub-packages, i.e. build_web_compilers, build_modules, build_analyzer_summaries? I feel like that's a little premature, and will make future syncing and version constraints more difficult, especially with planned changes for Kernel.

[natebosch]

I feel like that's a little premature, and will make future syncing and version constraints more difficult, especially with planned changes for Kernel.

In theory this will be helpful for Kernel because we'll add build_kernel package and move away from build_analyzer_summaries

We're also concerned about the version constraint propagation more challenging, but it's probably better than the alternatives.

Having build_node_compilers depend on build_web_compilers (and making some of it's internals public) would require rethinking how we apply builders from transitive dependencies and possibly new configuration surface.

Exposing node as an option to the build_web_compilers builders would definitely require new configuration surface.

Another nice thing about the separate packages is that they could potentially become useful in other ways. For instance if there ever becomes a benefit to the VM consuming kernel files the module and kernel packages could be put together for that. Or if we decide to revisit the idea of a summary backed Resolver (which could improve startup times for the first build in a fresh vm) we could use the module and summary packages.

[matanlurey]

@natebosch:

In theory this will be helpful for Kernel because we'll add build_kernel package and move away from build_analyzer_summaries

How about planning for that now, and just calling it build_kernel? Seems silly to create a package that we know we'll deprecate soon[ish]. Maybe there is a better word, or maybe this is appropriate enough just to go in build_modules - maybe build_modular? (I agree with it potentially being useful for the VM later on)

Having build_node_compilers depend on build_web_compilers (and making some of it's internals public) would require rethinking how we apply builders from transitive dependencies and possibly new configuration surface.

Maybe. Though couldn't build_node_compilers use parts of build_web_compilers without build_web_compilers being invoked? Or is that part of the problem here?

[matanlurey]

(Just trying to avoid this becoming pkg/test)

[natebosch]

Though couldn't build_node_compilers use parts of build_web_compilers without build_web_compilers being invoked? Or is that part of the problem here?

That's part of the problem here. Once build_web_compilers is in the transitive dependencies we'll want to run it's builders - but the builders it defines might conflict with the ones that build_node_compilers would want to define, or we'd be doing unnecessary work if we make sure that their file extensions are distinct.

I'm open to the idea of build_modules taking on both the module definition and the summary generation, and then later migrating it to kernel. @jakemac53 any opinion on that?

[matanlurey]

It's just not clear to me anyone would want to use build_modules and not kernel/summaries :)

[pulyaevskiy]

Please see #959 where I've submitted initial draft for splitting modules and summaries from build_web_compilers. I wasn't entirely sure if scratch space could be broken apart so I just left it in build_modules for now and wait for your feedback.

[jakemac53]

It's just not clear to me anyone would want to use build_modules and not kernel/summaries :)

That is fair - I am fine with us creating just build_modules today.

The only thing this prevents is mixing and matching analyzer versus kernel summaries.

[matanlurey]

@jakemac53:

The only thing this prevents is mixing and matching analyzer versus kernel summaries.

I could see that being a problem if Dart2JS is using Kernel and DDC is using analyzer. Maybe?

[jakemac53]

I could see that being a problem if Dart2JS is using Kernel and DDC is using analyzer. Maybe?

Yes, but I think we could split up the packages at that point if it does in fact become an issue