Proposal: PLATFORM.bundle / moduleIdFor about pal HOT 26 CLOSED

I'm just seeing this... I will think about it some more but my first impressions:

1. Annotating the code to be able to precisely trace dependencies is a daunting task for existing projects.
   If you want to split your app in several chunks, I guess that you need to be able to trace each file's dependencies and hence there is probably no other way.
   But for small to medium-sized apps that ship everything in one file, there should be an easier way to just include everything. The comment loader let you add a big fat glob at the top of your main file:

/* @import('./**\/*.ts') */
/* @import('./**\/*.html') */

And that was it. It would be nice to have a simple option like that for small projects.

2. I find the syntax large and ugly. My router config is already quite long, I don't really want to annotate it with more function calls. Could we use the string templates to help a bit here?

module`./main`

It would be very nice if the analyzer supported renaming the function at import, so that people who want it very short could alias it to mod or even just m.

Of course string templates support concatenation nicely, so globbing is doable.

This syntax is worse at code splitting, but nothing impossible, because in JS everything is possible :)

m('chunk-name')`./main`

from pal.

mdn is referring it as module-name so another alternative

PLATFORM.moduleName('module-name')

from pal.

@jods4 I disagree. PLATFORM.moduleName(...) should be the simplest possible API. The second, optional parameter of the moduleName function is an object, so we can extend that object's functionality to anything we like in the future.

But the core functionality, which is statically resolving module names should just be that. So to me, that part of the proposal (first parameter) is final and probably won't ever change. To achieve additional or custom functionality possible to analyze statically, we can create additional methods in the future, for instance PLATFORM.moduleInclude(/some-regex/), or whatever else you might want to add.

Let's not over-engineer this, let's keep it simple and expand as necessary and only when needs arise.

Do you have any alternative solutions in mind, @jods4? I'd love to hear any new ideas. The only viable alternative that I personally could think of is to list dependencies in the package.json as aurelia.build.dependencies, but that's been a maintenance mess so far. I've been thinking hard about this and came to the conclusion that it's:

• either listing dependencies centrally (like the package.json or aurelia.json approach)
• or listing them inside the files which require them (like this proposal)

The second option is easier to maintain, more portable and ensures the dependency graph is correct (cause we can trace which file included which dependency).

from pal.

@jods4

To address your concerns:

the current webpack setup is too complicated in several ways

I agree. This proposal is part of the solution to this problem. With it we won't need to manually specify build resources inside aurelia.build.resources, it's been a headache, a source of problems and can be made redundant or unnecessary.

Also, please keep in mind that that this is not a Webpack-specific solution and will enable portability across multiple build systems. Might also make it possible to simplify aurelia-cli configuration, and other, custom build configurations.

(a) do we really need an Aurelia abstraction for that? Why re-invent something that is already natively supported? If I have to instrument my router, I can do so today without this plugin, with better build perf.

Yes, we do. With the current design of aurelia-loader, an abstraction for any kind of a loader, loading modules is essentially a type of dependency injection, where the modules resolved by the loader are the dependencies being asynchronously injected. The keys are module names and the values are resolved modules. Thus, the challenges are also pretty similar to those which appear with DI, but complicated by the fact that no official loader standard exists on the web today, and an abstraction is required. Further complicated by the fact that the module names (keys) are strings that can be concatenated or generated dynamically. This is not a trivial problem to solve.

Due to the nature of JavaScript - i.e. it being a non-typed language, this presents both opportunities and trade-offs.

As for your "build performance argument": build performance will not influenced by this, also when using Webpack, because Webpack always parses the JavaScript before emitting it. In case of other types of builds, it will be possible to do the same in a simple babel plugin (so again, the build time won't be effected much, since Babel already has the parsed AST present).

What do you mean by "natively supported"? The only browser that supports ES6 modules natively is Safari Technical Preview (as of last week).

(b) and probably at the very opposite: how much can we automatically infer as a dependency without an additional function? Quite a lot I think, which would remove the need for code migration, prevent omissions and keep the code clean.

I have considered inferring dependencies via complex AST analysis and have rejected this idea.
First of all, programming such a static analysis function would be very complicated and time-consuming. It would also be error-prone and a huge burden to maintain (any new APIs would need to be added to the static analyzer, or they would cause errors for users). In other words, it would be very brittle, and I don't think brittle code is a good idea. The proposal might add a little verbosity, but its a very small trade-off compared to the alternative.

But as I said in my previous comment, if you have alternative ideas on how to tackle this, I'm all ears. If not, I'd like to move forward with this ASAP, so that users can start using the new functionality, especially since we have 8 positive votes under this proposal and yours is the only critical one.

from pal.

I had an interesting discussion with @niieani in gitter, so I'll sum up some of our discussion so that others can benefit from the information as well. There might be a few things slightly out of topic for this issue but I think it's still interesting.

1. Aurelia really wants some standard way to declare dynamic dependencies (e.g. routes, global resources, dynamic views) so that an Aurelia libs can be easily consumed by everyone, with any bundler.
2. The larger part of that plan is that there needs to be "plugins" or pre-processors for each bundler, that collect those dependencies. This can be with very little perf cost if there is a way to tap into the compilation pipeline that is happening anyway (e.g. if you use webpack or babel), or it could be a pre-processor step that produces a list of dependencies (e.g. for requirejs) in which case there would be an extra parsing step. We did not discuss which build systems will be supported but it seems clear to me that webpack is the first target, and babel should not be too far as the plugin systems are mostly compatible.
3. A very important aspect of the new approach is that it doesn't add new concepts in the build tool. PLATFORM.moduleName is an identity function (if it's not removed at build time). When using the build plugin, that call will be removed and a normal dependency will be registered in the compilation process.
4. The last point means that in your own code you can use alternative means if you prefer. For example, if your application is not chunked, you can simply add everything as a dependency and be done. You lose static verification of your module strings, but that's something you should test for anyway.
5. Alternatively, you can write your own plugin that uses a different convention to enlist dependencies. Maybe the official plugin will support shorter aliases (not discussed) but if not the fully custom route is possible.
6. Using multiple functions to detect dependent modules was judged confusing, for example calls to @useView, globalResources or feature could easily register a dependency in the same way moduleName does. But the full .globalResources(PLATFORM.moduleName('./custom-element')) is the only supported way. I don't agree fully here, but at least I'm free to use my own plugin if I want to, see 5.
7. I noted that annotating all dependencies is a lot of work and error-prone for large existing projects, (in favor of 6.) so that shortcuts might be appreciated but that wasn't deemed a good enough argument.

Then the discussion shifted to some complexities of the current webpack skeleton, so a bit unrelated.
I pointed that RewriteModuleSubdirectoryPlugin is mostly required just because some Aurelia libraries are distributed as packages (e.g. aurelia-templating-resources), which is annoying with most bundlers. I am not sure why there are 2-3 libs like that, but getting rid of this would be nice.

RootMostResolvePlugin in the skeleton is not strictly needed but it may help when different libs depend on different versions of another lib, as NPM support multiple versions of modules. @niieani says that NPM 3+ or Yarn, that flatten dependencies as much as possible, help but do not completely resolve the issue.

MappedModuleIdsPlugin is used to keep the original modules names in place, so that Aurelia can easily load them -- normally webpack uses plain integers. I point the complexity of the 20+ lines of config for a "default" behaviour and @niieani says he'll try to provide presets to ease the use in common cases.

The list-based-loader and comment-loader from current skeleton should go away.

And for a final wish: I note that starting a plain web project with webpack is really easy and makes for an awesome experience. Currently adding Aurelia to that basic setup is painful, I hope we'll be able to make that as smooth and easy as possible. 🎄

from pal.

@niieani I have mixed feelings about 2.

On one hand many people including myself would like to use webpack, so getting this out is important.

On the other hand, this feels rushed and not mature enough. It's at least the 3rd iteration (that I know of) in a relatively short amount of time. When this is out, some users will start sprinkle PLATFORM.moduleName(...) all over their code. It takes time, so we'd better be sure this is the solution we want in the long run. I also fear that once a solution is out, it will hamper the development of alternative solutions.

from pal.

@niieani @EisenbergEffect
We need to discuss the first API extension of moduleName...

Right now the webpack plugin supports:

• moduleName('mid'), which defines a module id for consumption by aurelia-loader;
• moduleName('mid', 'chunk'), which does the same but creates a split point, allowing mid to be loaded and executed only when required.

The extension plan is moduleName('mid', { option: value }).

I thought we could do without extensions for now but I already have a need... ☹️

It's related to tree shaking. A moduleName dependency assumes that all exports are used. 99% of the time, especially with resources or views, this is totally fine.
But in the rare cases where the module exports a lot and you know you won't use much it would be nice to specify precisely what you're going to use, so that tree shaking can potentially remove the unused code.

Very important case: aurelia-framework is dynamically imported by aurelia-bootstrapper, only to grab the Aurelia class. But aurelia-framework basically re-exports everything in the core of Aurelia. So that single dependency is responsible for preventing tree shaking of almost everything in the framework 😭

To get at least some tree shaking happening, I want to do:
moduleName('aurelia-framework', { exports: [ 'Aurelia' ] }).

As usual the debate will be about naming, I suggest:

{
  chunk: 'chunk name, currently passed as a plain string',
  exports: [ 'array of used exports' ]
}

What do you think?

This does not impact the PLATFORM implementation, but it should be integrated in the TS definition.

from pal.

That sounds like a reasonable api to me.

from pal.

I agree, this is a good addition @jods4 and can potentially be consumed by other build systems (e.g. rollup). I say we go ahead and add this. We can think of deprecating this in Aurelia 2.0 if we rework module loading a little bit, but for now this seems acceptable. 👍

from pal.

It would be great to have an updated section in the doc informing end users how to create tree-shaking friendly instruction with new version

@JoD @niieani

from pal.

I like this. I'm wondering about the api name being bundle which doesn't seem quite right since it's a module id. If we can tweak that a bit, I think I'd say "go for it".

from pal.

Yeah, I'm not sure about the API name either, bundle() is pretty much a placeholder.

Some alternatives I came up with:

• PLATFORM.bundle('some-module')
• PLATFORM.moduleIdFor('some-module')
• PLATFORM.getModuleId('some-module')
• PLATFORM.normalize('some-module')
• PLATFORM.id('some-module')
• PLATFORM.module('some-module')

But I'd love some more / better ideas.

from pal.

Great reference there. Back in the require.js days I think the term was moduleId, which is what we used in Durandal and then was inherited in the appropriate Aurelia APIs. We could use the term moduleName in the new PAL API and then go back and create property getter/setter aliases on all the existing APIs so that we could have consistency, if we want, without breaking anything. Thoughts?

from pal.

That sounds good, Rob. I'll go ahead and start working on the implementation using the moduleName function, so that we'll be able to use that for static analysis instead of the comment approach in the new experimental Webpack skeleton. I think it'll be better to start using this, so there will be no need for any migrations down the line.

Later on we can alias moduleName to moduleId in other places, like the router, as you suggested.

from pal.

Just checked, ECMAScript 2015 document actually refers to module names as ModuleSpecifiers. But I think the MDN's module-name is still a better name.

from pal.

@jods4 you'll be able to achieve 1 using conventional-loader, without the ugly comments, by defining the regex explicitly in the Webpack config.

As for 2, renaming and template strings can be added in the future, after we discuss this further. For now, let's just get this out so that people can start using it.

from pal.

@niieani I'm not trying to over-engineer. In fact I think the current webpack setup is too complicated in several ways.

Two things I'm currently thinking about are:
(a) do we really need an Aurelia abstraction for that? Why re-invent something that is already natively supported? If I have to instrument my router, I can do so today without this plugin, with better build perf.
(b) and probably at the very opposite: how much can we automatically infer as a dependency without an additional function? Quite a lot I think, which would remove the need for code migration, prevent omissions and keep the code clean.

from pal.

Thanks @jods4 for the nice summary. It will be helpful in writing the guide explaining both this proposal and new skeleton in a future blog post.

from pal.

@jods4

annotating all dependencies is a lot of work and error-prone for large existing projects, (in favor of 6.) so that shortcuts might be appreciated but that wasn't deemed a good enough argument.

If You propose shortcut for PLATFORM.moduleName( because You are afraid that You'll make a typo in writing it out, then this is where TypeScript compiler will produce a compile-time error (if You use TypeScript instead of Babel). So I probably won't miss a shortcut for the method call.

from pal.

@atsu85 no, I was proposing that a call to globalResources() is considered as a dynamic dependency the same way PLATFORM.moduleName is.

The "work" referred to adding the annotation when it could be inferred and the "error-prone" referred to forgetting to do so/missing an existing call in a large existing app.

BTW I am a TS user 100%, with all the safety options turned on.

from pal.

@niieani @EisenbergEffect Any update on this? It seems it's been dead in the water for a few weeks. @jods4 has it working nicely on his repo.

from pal.

The API has already been merged into PAL. I believe that @niieani is still working on the Webpack plugins that use it. We're working with @jods4 to build the solution.

from pal.

@EisenbergEffect I have also been talking with @jods4 about this feature and before I attempted to bake in his solution, I wanted to check with @niieani and see where he is with this. If it's not going to be ready any time soon, I'll start migrating @jods4 code into my build.

from pal.

Some unexpected work came up on my side, so I had less time to finalize this.

I think joining forces with @jods4 is a good idea, since it will allow for faster development, especially when one of us is unavailable. @jods4 also had some good ideas. I already spoke with him about it and I'd like to ask @EisenbergEffect to approach him too, whenever possible.

from pal.

I bumped into an issue with the plugin that walks PLATFORM.moduleName expressions.
It works great in code that uses ES modules, but when transpiled to other systems, e.g. commonjs, the code becomes:

var _aureliaPal = require("aurelia-pal");
_aureliaPal.PLATFORM.moduleName("whatever");

So basically, I've had to accept any occurrence of xxx.PLATFORM.moduleName() in addition to just PLATFORM.moduleName().

from pal.

Merged into pal. Releasing update shortly.

from pal.