Skip to content
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

Switch Loader to AMD or import #143

Open
greggman opened this issue Jan 4, 2017 · 7 comments
Open

Switch Loader to AMD or import #143

greggman opened this issue Jan 4, 2017 · 7 comments

Comments

@greggman
Copy link
Contributor

greggman commented Jan 4, 2017

Right now there's custom loaders in core/embed.js and core/loader.js. If I switch to AMD style then I think I can get webpack to do the build automatically. Webpack follows the require calls to find all the included modules which means the build script doesn't have to be updated when new things are added. AMD style can also still be used without compiling as well so that seems like a win

I can also switch to use ES style import but that can't be used without a compile step.

Thoughts? Go AMD? Go Import? Leave it as is?

@benvanik
Copy link
Owner

benvanik commented Jan 4, 2017

If Chrome supported import that'd be best (as woo future!), but being able to run uncompiled is nice so AMD is probably best for now. It should be easy enough to switch in the future when browsers start supporting import (I bet someone will have a tool for it :)

@greggman
Copy link
Contributor Author

greggman commented Jan 5, 2017

Ok, I have should have a PR in a day or 3 that's AMDed. It touches every file and sadly it's after my WebGL2 work so far. Sorry it's not all tiny PRs

One issue I'm having though is AMD at least using require.js loads more async. I might be able to hack it. but ..

  1. Because require.js loads async window.onload gets called before embed.js has had a chance to wrap getContext.

    Note: This should only be an issue for the gliEmbedDebug version.

    I might be able to solve that by hacking require.js or writing my own. Otherwise anytime you use the embedded debug version in an app you'd need to do something to get it to run after embed.js has wrapped getContext. I'm not sure how big a deal that is. Assuming embed.js debug's only real point is debugging maybe that's not an issue? Like I could make embed.js emit an event on the window maybe as one solution if I can't get it to work the same as the current embed

  2. Because it's using require.js I have no idea what issues will come up if the app you're trying to use embed.js in was already using require.js. Hopefully none but if I have to hack require.js to fix Blending/depth test broken #1 above that might be more of an issue. Again I think this only effects when in debug mode (the mode that loads every file separate)

@benvanik
Copy link
Owner

benvanik commented Jan 5, 2017

Restrictions like that in debug mode are fine I think. When packaged it should still work, and that's how most people use it (I'm hoping! debug is slow!)

@benvanik
Copy link
Owner

benvanik commented Jan 5, 2017

Do you think pull #137 will be solved by this AMD change? Or will what it does be even more required?

@greggman
Copy link
Contributor Author

greggman commented Jan 6, 2017

So just to double check I tried an AMD program

As I suspected you can't use the debug version of the WebGL-Inspector
with AMD programs easily. The non-debug versions work fine so I don't think this is an
issue. I posted an example here

twgl-amd.html                 // a normal AMD app
twgl-amd-embedded.html        // a version that uses embed.js
twgl-amd-embedded-debug.html  // a version that uses embed.js in debug mode

All of them work in non-debug mode. The last one shows how to get embed.js
to work in debug mode with AMD. Unfortunately the debug version of the extension
doens't work period and I can't think of any easy work around.

Like I said I don't think that's an issue but I thought it was best
to at least check the non-debug version works and document how to
get the embed debug version working

@greggman
Copy link
Contributor Author

greggman commented Jan 6, 2017

Oh yea, and FYI I effectively reverted #116, not all of it, just the parts on HostUI.js, TraceView.js and TraceTab.js.

@greggman greggman mentioned this issue Jan 10, 2017
Merged
@benvanik
Copy link
Owner

Reverting #116 sounds fine to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants