liferay / liferay-amd-loader

GNU Lesser General Public License v3.0
18 stars 20 forks source link

Non deterministic behaviour #90

Closed yuchi closed 7 years ago

yuchi commented 7 years ago

I’m having a hard time in finding a minimal reproducible test case.

We currently have two main issues.

1. Order of import matters

Given a MyButton.js:

import Soy from 'metal/…/Soy';
import templates from './MyButton.soy';
export default class MyButton { }
Soy.register(MyButton, templates);

and a MyApp.js:

import Soy from 'metal/…/Soy';
import templates from './MyApp.soy';
import MyButton from './MyButton';
export default class MyApp { }
Soy.register(MyApp, templates);

soy will break (I’ll post the error soon).

I need to modify MyApp.js into:

import Soy from 'metal/…/Soy';
import templates from './MyApp.soy';
import _ from './MyButton.soy'; // <--- notice how I need to pre-import the templates
import MyButton from './MyButton';
export default class MyApp { }
Soy.register(MyApp, templates);

2. Missing exports

Sometimes, non deterministically, a module will export an empty array [] instead of the correct object. This happens very often on Soy templates.

yuchi commented 7 years ago

Obviously this is not enough to reproduce the issues and solve them.

Let me know how can we do to fix this behaviors.

julien commented 7 years ago

Hi @yuchi,

Just a quick question, in your first example (MyButton.js) is it normal that it does not extend (Metal) Component?

yuchi commented 7 years ago

Sorry just super reduced to uncluttered the example. It is obviously a metal component.

julien commented 7 years ago

Ok, no worries, I just wanted to make sure I understood correctly. thanks

jbalsas commented 7 years ago

Hey @yuchi, we are doing that exact same thing everywhere... only the components import their templates, no need to prefetch templates from other components, so there's something weird going on.

Could you maybe share the generated config.json file for that bundle as well as the processed define(... instructions?

About 2, I think that's caused by the build system running transpileJS but not configJSModules due to some gradle cache issue... as a workaround, you should be able to remove the classes folder and run gradlew ... again.

If I'm not mistaken, we did fix this, so maybe you just need to update to the latest released plugins? Which version are you currently using?

yuchi commented 7 years ago

config.json (formatted and clean up):

{
  "my.bundle@1.0.0/my_stuff/MyApp.es": {
    "dependencies": [
      "exports", "metal-component/src/Component", "metal-soy/src/Soy",
      "metal/src/core", "./MyApp.soy", "./MyButton.soy", "./MyButton.es"
    ]
  },
  "my.bundle@1.0.0/my_stuff/MyApp.soy": {
    "dependencies": [
      "exports", "metal-component/src/Component", "metal-soy/src/Soy"
    ]
  },
  "my.bundle@1.0.0/my_stuff/MyButton.es": {
    "dependencies": [
      "exports", "metal-component/src/Component", "metal-soy/src/Soy",
      "metal-events/src/EventEmitter", "metal/src/core", "./MyButton.soy"
    ]
  },
  "my.bundle@1.0.0/my_stuff/MyButton.soy": {
    "dependencies": [
      "exports", "metal-component/src/Component", "metal-soy/src/Soy"
    ]
  }
}

Definitions (formatted and cleaned up):

// MyApp.es.js

import Component from 'metal-component/src/Component';
import Soy from 'metal-soy/src/Soy';
import core from 'metal/src/core';

import componentTemplates from './MyApp.soy';
import buttonTemplates from './MyButton.soy';

import MyButton from './MyButton.es';

// becomes
define("my.bundle@1.0.0/my_stuff/MyApp.es", [
  'exports', 'metal-component/src/Component', 'metal-soy/src/Soy',
  'metal/src/core', './MyApp.soy', './MyButton.soy', './MyButton.es'] /*...*/ );
// MyButton.es.js

import Component from 'metal-component/src/Component';
import Soy from 'metal-soy/src/Soy';
import EventEmitter from 'metal-events/src/EventEmitter';
import core from 'metal/src/core';

import templates from './MyButton.soy';

// becomes
define("my.bundle@1.0.0/my_stuff/MyButton.es", [
  'exports', 'metal-component/src/Component', 'metal-soy/src/Soy',
  'metal-events/src/EventEmitter', 'metal/src/core', './MyButton.soy'] /* ... */)
yuchi commented 7 years ago

Extracted from settings.gradle

buildscript {
  dependencies {
    classpath group: "com.liferay", name: "com.liferay.gradle.plugins", version: "3.0.17"
    classpath group: "com.liferay", name: "com.liferay.gradle.plugins.workspace", version: "1.1.0"
  }
}

There’s nothing special to extract from my-bundle/build.gradle.

ipeychev commented 7 years ago

Maybe @jbalsas could help you here, Pier. However, If you manage to provide reproducible code out of Liferay, I will take a look.

yuchi commented 7 years ago

Hi @ipeychev I’m trying hard in order to get a test case. I opened this issue in order to keep track of it and see if it tingled something in @jbalsas mind while I’m busy building a minimum environment that I can share.

jbalsas commented 7 years ago

Hey guys, sorry but I was on EVP for the day. Will take a closer look tomorrow! El El mié, 23 nov 2016 a las 20:12, Pier Paolo Ramon < notifications@github.com> escribió:

Hi @ipeychev https://github.com/ipeychev I’m trying hard in order to get a test case. I opened this issue in order to keep track of it and see if it tingled something in @jbalsas https://github.com/jbalsas mind while I’m busy building a minimum environment that I can share.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/liferay/liferay-amd-loader/issues/90#issuecomment-262605709, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3PLmVbZLz3duNpkgHrn0zs5h8gNo_Aks5rBJAmgaJpZM4K5Qtr .

jbalsas commented 7 years ago

Hey @yuchi, I forgot to ask... could you also share the details of the configuration that is not working for you?

If you prefer (and are able to), I'd also take the bundle to try it out myself 😉

yuchi commented 7 years ago

@jbalsas Once I have the permissions I'll send you the sources in a private channel.

jbalsas commented 7 years ago

Hey @yuchi, not sure if we ever got to fix this... are you still experiencing it? Should we close it, or take a new look?

Maybe @izaera can take care of it if you're still seeing this...

yuchi commented 7 years ago

I didn't have the time to create a test case. I employed a pattern that mitigated the issue and I have been following it since then, so I cannot fully tell it's gone or not.

The pattern BTW is simply to require only a single endpoint inside aui:script and inside that import stuff. Also never import a component without first importing its template.

Not nice but worked.

jbalsas commented 7 years ago

Hey @yuchi, that's interesting! You didn't mention the aui:script part before... do you think it is relevant? If so, the issue will likely be related to Liferay Portal...

Is it okay if we close this one and keep an eye open in case it resurfaces?

Thanks!

yuchi commented 7 years ago

Yeah go ahead and close it. Anyway it was related to code calling require, since once inside defined modules more or less everything worked.

If I have a test case I'll open again.

jbalsas commented 7 years ago

Awesome, thanks for everything, @yuchi!!