New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use aurelia-loader instead of hardcoded systemjs loader #233

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@Zensavona
Contributor

Zensavona commented Mar 31, 2016

This PR attempts to fix #226, but comes with some caveats - if you guys (@EisenbergEffect, feel free to chime in here) have any suggestions to improve this, I'm more than happy to help, but given the architecture of this module these are the best solutions I could come up with given my somewhat limited understanding.

  1. A new window global (window.loader) has been introduced in index.js because there seems to be no straightforward way of getting a singleton instance of Loader inside ValidationLocaleRepository using DI.
  2. The dependencies section of package.json has been removed - I don't really understand why there are double dependency declarations (npm and jspm via npm) here? But I've found that whenever dependencies are declared there, aurelia.container.get(Loader) gives me a fresh (non instantiated and useless) Loader instance, while when dependencies are only declared within the jspm area, I get the expected DI'd instance. I'd really like to understand what's going on here. If someone can enlighten me I'd greatly appreciate it.
  3. A test case (on using decorators to set up validation on a single property that is valid) is failing, but it fails on master also.

Thanks!

P.S. I will add (thanks @stoffeastrom!) for this to work with webpack, the following configuration in webpack.config.js is required:

plugins: [
    new AureliaWebpackPlugin({
      includeSubModules: [
        { moduleId: "aurelia-validation" }
      ]
    })
  ]
@valichek

This comment has been minimized.

valichek commented Apr 8, 2016

@Zensavona Using window.loader global is not good, so hope this solution is considered as temporary

@valichek

This comment has been minimized.

valichek commented Apr 8, 2016

It's better option to expose loader to plugin config, something like in aurelia-i18n, @EisenbergEffect what do you think, so loading of resources could be highly customized
Here is my example for aureli-i18n and webpack https://gist.github.com/valichek/5dc7c1e11ed39a82df83913df465cb50

@EisenbergEffect

This comment has been minimized.

Member

EisenbergEffect commented Apr 8, 2016

Yes, it should happen through the plugin config. The plugin itself has access to the loader and can use it at that point. This works especially well since plugin configure functions can return a promise. So, the plugin can allow configuration and then perform async loading.

@PWKad Is there any chance we can get a fix for this? Because this is written incorrectly, no one can use validation with webpack.

@PWKad

This comment has been minimized.

Contributor

PWKad commented Apr 9, 2016

I've been digging in to this to understand the problem more today. It's not as simple as waiting until the configure method because of the way everything is setup. The ValidationLocaleRepository is new'ed up before the configure method is called and it's currently not using DI in there in any way.

I was planning on trying to finish up the new validation library this week but I'm going to redirect focus to learning more about how the current validation library works and then trying to refactor it to support other loaders.

@Zensavona

This comment has been minimized.

Contributor

Zensavona commented Apr 10, 2016

@valichek I'm aware it's not ideal, as I detailed in the PR - it's written this way because of what @PWKad just mentioned - this is a "make it work" fix, not a permanent solution. I didn't have time to fully investigate the way all of the moving parts around this work honestly, I just needed it to work.

P.S. @PWKad & @EisenbergEffect any insight on why having dependencies in the top level area of package.json could cause a new instance of Loader being inserted as I mentioned in the original PR?

@masterpoi

This comment has been minimized.

masterpoi commented Apr 11, 2016

FWIW and if i understand this correctly, Webpack 2 will support/recognize System.import : https://gist.github.com/sokra/27b24881210b56bbaff7

@EisenbergEffect

This comment has been minimized.

Member

EisenbergEffect commented Apr 11, 2016

That's actually another issue because System.import isn't the standard API anymore. Someone should probably tell them it is changing. Hopefully it's easy for them to adapt. Maybe they need a generic means of identifying dynamic module load code?

@EisenbergEffect

This comment has been minimized.

Member

EisenbergEffect commented Apr 11, 2016

I'm closing this for now. I believe @PWKad is going to submit another PR that will be a bit more correct.

@PWKad

This comment has been minimized.

Contributor

PWKad commented Apr 11, 2016

#236 @Zensavona same concept just using DI to grab it. Can you and / or @valichek verify that PR handles all scenarios that you see being blocked currently before I merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment