search

aurelia / webpack-plugin

A plugin for webpack that enables bundling Aurelia applications.
MIT License
90 stars 36 forks source link

Imported classes shouldn't need PLATFORM.moduleName #151

Closed zakjan closed 6 years ago

zakjan commented 6 years ago

I'm submitting a bug report

Current behavior:

Imported classes passed into Aurelia libraries needs to be imported again with PLATFORM.moduleName for production bundle.

Steps to replicate:

$ au new test
$ cd test
$ npm i aurelia-dialog

Edit files:

src/app.ts

import {autoinject} from 'aurelia-framework';
import {DialogService} from 'aurelia-dialog';
import {DialogViewModel} from './dialog';

@autoinject
export class App {
  message = 'Hello World!';

  constructor(private dialogService: DialogService) {}

  openDialog() {
    this.dialogService.open({
      viewModel: DialogViewModel,
      lock: false
    });
  }
}

src/app.html

<template>
  <h1>${message}</h1>

  <button click.trigger="openDialog()">Open Dialog</button>
</template>

test/unit/app.spec.ts

import {App} from '../../src/app';

describe('the app', () => {
  it('says hello', () => {
    // @ts-ignore
    expect(new App().message).toBe('Hello World!');
  });
});

src/dialog.ts

export class DialogViewModel {
    message = "Hello World!"
}

src/dialog.html

<template>
    <ux-dialog>
        <ux-dialog-header>
            Dialog
        </ux-dialog-header>
        <ux-dialog-body>
            ${message}
        </ux-dialog-body>
    </ux-dialog>
</template>

Run development bundle:

$ nps

Verify that dialog can be opened.

Run production bundle:

$ nps webpack.build.production.serve

The dialog can't be opened, it throws:

bluebird.js:1545 Unhandled rejection Error: Can not resolve "moduleId" of "".
    at e.ensureViewModel (http://localhost:8081/app.e387fbb6ae0377584acf.bundle.js:1:306249)
    at e.open (http://localhost:8081/app.e387fbb6ae0377584acf.bundle.js:1:307634)
    at t.openDialog (http://localhost:8081/app.e387fbb6ae0377584acf.bundle.js:1:353245)
    at e.evaluate (http://localhost:8081/app.e387fbb6ae0377584acf.bundle.js:1:223617)
    at t.callSource (http://localhost:8081/app.e387fbb6ae0377584acf.bundle.js:1:289885)
    at t.handleEvent (http://localhost:8081/app.e387fbb6ae0377584acf.bundle.js:1:290046)

Edit src/app.ts, add lines:

import {PLATFORM} from 'aurelia-pal';
PLATFORM.moduleName('./dialog');

Therefore now there is both ES native import and Aurelia Webpack Plugin import:

import {DialogViewModel} from './dialog';
...
PLATFORM.moduleName('./dialog');

Verify that now the dialog can be opened.

Similar behavior exists when a class reference is used in @viewResources or <compose view-model.bind="..."></compose>.

Initially this would affect aurelia-dialog view models only, but now as Aurelia is moving towards more support for class references instead of string paths (https://aurelia.io/blog/2018/06/24/aurelia-release-notes-june-2018/#new-templating-features), this issue becomes more widespread.

Expected/desired behavior:

shabalin commented 6 years ago

Similar happens when you declare component dependencies via @viewResources. Example: 

import {Component} from "./component";
PLATFORM.moduleName("./component");
@viewResources(Component)
export class AnotherComponent {
}

If you remove PLATFORM.moduleName("./component"); you'll have "module ... component.html not found"

jods4 commented 6 years ago

This happens because moduleName does more than just add a dependency. It also prevents Webpack from optimizing your module in ways that break aurelia-loader, such as: renaming your module, hoisting into another module, tree shaking unused exports and more.

When you ES import your module Webpack will bundle it into your application, but it will also do all those things, which prevent aurelia-loader from finding the requested module.

So there's nothing we can do about it.

On the upside, @bigopon is refactoring Aurelia APIs to avoid usage of aurelia-loader in favor of standard import (static or dynamic), which means we can progressively get rid of moduleName and aurelia-webpack-plugin, eventually.

zakjan commented 6 years ago

@jods4 This is understandable with string paths, because there is no real import. But with imported classes, it gets confusing.

What's the general rule when it is necessary to use moduleName? Is there any automated way how we could check for missing moduleName and prevent such production-only runtime errors?

The refactoring sounds good, can you point me to the tasks where it is being done, so that we can track the progress?

jods4 commented 6 years ago

@zakjan I agree that it's confusing, sadly without refactoring the libraries there's nothing that can be done in a Webpack plugin that would fix this.

Thankfully, passing an object to an API that will later try to fiddle with its aurelia-loader origin is rather rare. aurelia-dialog is certainly the most commonly used example.

An automated way to check those would be to have a JS analyzer that has a list of "sensitive" APIs and tracks down the source of their parameters (to the extend that it's possible...) and double-check if there's a matching moduleName. It would be a very complex projecta and it would never be perfect.

I hope that you always do acceptance tests of your systems with the same build that you intend to put in production, which should catch those errors as they are not subtle.

I think the refactorings are doing well, if I'm not mistaken a lot have been published already. I'll leave it to @bigopon for the details.

CC @StrahilKazlachev anything to add when it comes to aurelia-dialog?

StrahilKazlachev commented 6 years ago

For >=aurelia-dialog@1.0.0 when it comes to using a class for viewModel you will need to have PLATFORM.moduleName somewhere. From the issues in dialog I remember that more people preferred adding it in the module of the VM itself - you do need it only once.

I can't guarantee whether it will be this week or not, but we will push out 2.0.0 of the dialog. It will have the same APIs as 1.0.0, excluding the configuration API(the plugin config one!!). And it requires ^aurelia-templating@1.8.2. The commit to drop the usage of Origin is already pushed. The only thing left is how to proceed with the default resources - <ux-dialog> & Co. Will open a PR with proposition this week, I hope.

zakjan commented 6 years ago

@jods4 Thanks. Sure we have QA to catch this, but it would be better to catch it earlier in the development process. The current list of sensitive APIs would be:

Is there anything else?

bigopon commented 6 years ago

@zakjan that would be all for now. Router moduleId: () => MyVmClass is on the list but it will probably take bit more time. Note that @viewResources relies on Origin, otherwise you need to use absolute path:

@viewResources(PLATFORM.moduleName('my/absolute/path')) // if MyVm is referenced statically
export class MyVm {

}