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

Create factory shortcut throws error in IE8 #140

Closed
gehan opened this issue Nov 6, 2013 · 34 comments
Closed

Create factory shortcut throws error in IE8 #140

gehan opened this issue Nov 6, 2013 · 34 comments
Assignees
Milestone

Comments

@gehan
Copy link

gehan commented Nov 6, 2013

If you do this

create: 'app/conversations/utils/UserProxy'

instead of

create: 
    module: 'app/conversations/utils/UserProxy'

Then IE8 throws a 'Type Error: Object expected'

@briancavalier
Copy link
Member

Yikes! I'll look into this asap. Thanks, @gehan

@gehan
Copy link
Author

gehan commented Nov 6, 2013

Yeah was throwing me for a while!

Not sure if it affects using shorter factory definitions anywhere else, but def for create!

@briancavalier
Copy link
Member

Silly question, but are you using cujojs/poly (or another ES5 polyfill)? I figure you are, but just want to rule out the obvious :)

@gehan
Copy link
Author

gehan commented Nov 6, 2013

I'm using poly/array, poly/object, poly/function !

@briancavalier
Copy link
Member

Ok, thanks. I'm able to reproduce the problem, but as usual with IE, tracing to find the actual cause is painful :) Working on it tho. If you come across any more info, let me know!

briancavalier added a commit that referenced this issue Nov 6, 2013
@briancavalier
Copy link
Member

Hey @gehan, I just pushed a potential workaround to the fix-140 branch. Could you give it a try and let me know if it works for you?

This is a pretty baffling problem. While the workaround does work for me, I don't yet know why, or what the actual root cause of the problem is in IE8. For some reason trying to apply a constructor function in this particular case fails, even though, by all observable accounts (using IE8's awful dev tools), everything is identical about the constructor and its prototype in my test case.

@gehan
Copy link
Author

gehan commented Nov 6, 2013

Yeah, IE is super weird. I'll give it a go!

Not other wire problems... however we're trying to debug why IE8 isn't working in our site. However since a lot of things get fired off inside promises... they swallow all the exceptions even if they are unhandled.

I know this is a known issue and expected behaviour but couldn't we have a debug option in when.js to expose all exceptions or at least log then anyway?

@briancavalier
Copy link
Member

Have you tried enabling when.js's unhandled rejection monitor?

@briancavalier
Copy link
Member

Oh, and if this workaround works for you, I'll include it in a wire release this week :)

@gehan
Copy link
Author

gehan commented Nov 6, 2013

@briancavalier yes thats exactly what I was looking for - AWESOME

Is that safe to use in production?

Or rather I'd love to capture all unhandled exceptions and log them to the console in dev and our Sentry/exception server in prod!

@briancavalier
Copy link
Member

I definitely wouldn't use it as-is in production :) It's purely intended for dev/debug time, and introduces some significant performance overhead.

However, the interesting thing about the monitor is that it's an API. You could write your own and when.js will call it. So, for example, you could probably create a lighter weight version that doesn't do the extended stack trace stitching, or defers collecting stacks and reporting rejections until the app is idle, etc.

If you want to go in that direction, just let me know, and I'd be happy to answer questions, etc.!

@briancavalier
Copy link
Member

Another thing you can do is, if you're using wire as an AMD plugin, always add an AMD error handler to your top-level AMD bootstrap. For example, if you're using curl:

curl(['wire!myapp/main'], function(mainContext) {
   // maybe nothing here
}, function(error) {
   // Report fatal errors
   console.error(error);
});

If the AMD loader supports that (curl does, and I think requirejs does, too), wire will hook into it. So, anything that causes a wiring failure will bubble up to the error handler. I'm also working on a related change that will report a loud stack trace when the AMD loader doesn't support an error handler.

@gehan
Copy link
Author

gehan commented Nov 6, 2013

Yep for wire specs I've always added the fail handler! Works great

I see you've added (in master) promise.done, which should sort it out too right? Is that for when 2.6?

@briancavalier
Copy link
Member

Yeah, in fact, I (quite literally) just published 2.6.0 :) It does have promise.done, and it will help if we use it in some strategic spots. We probably have to give a little time for people to update before we can use it in wire.

I made some other changes in wire that I'm planning to release alongside this workaround for create.

@gehan
Copy link
Author

gehan commented Nov 6, 2013

Ha and I was literally just looking for it! I'll try now.

I think done is the good move, aside from having something to monitor unhandled exceptions. I think it's safe to know when you're promise chain is done. I'm not sure how this would work with promise connections in wire though..?

@gehan
Copy link
Author

gehan commented Nov 8, 2013

I tried it on IE8 and works fine!

@briancavalier
Copy link
Member

Cool, thanks @gehan. I probably won't have time today to pull a release together, but I should be able to get it out early next week.

@gehan
Copy link
Author

gehan commented Nov 8, 2013

Slacker! ;)

What's new in 0.11 then? Relative paths for plugins in specs too?? ;) Actually that was dependant on the curljs extension loader changes right?

@briancavalier
Copy link
Member

Yeah, we were going to try to solve that issue by implementing extension-based loading in curl. It might be good to provide some level of functionality in wire as well, though, for people who still explicitly use plugins (or can't immediately updated their specs to rely on extension-based), or another AMD loader.

I've been thinking that a simple approach might be to split the module id on '!'. If there are 2 parts, and the second part starts with './' or '../', assume it's path-like and run the same relativizing algorithm that wire uses for non-plugin module ids now, then stitch the complete id back together. @unscriptable, any thoughts on that?

@unscriptable
Copy link
Member

I keep thinking that the AMD loader's id mapping/transform behavior would conflict with your relativizing behavior. But it won't, afaict. All that wire.js will do is normalize the id -- and that is universal across loaders, afaik, since it's defined in the AMD and CommonJS specs. If we start interoperating with other module systems that don't normalize the same way, then we might hit a problem.

tl;dr: lgtm.

@briancavalier
Copy link
Member

Hey @gehan, see 950556a :)

@gehan
Copy link
Author

gehan commented Nov 10, 2013

Awesome @briancavalier ! Just tried it and works like a charm ;)

@gehan
Copy link
Author

gehan commented Nov 10, 2013

Although I guess you'd need to write something for cram to get that working right?

@unscriptable
Copy link
Member

This should work fine with cram.js. We're relying on the fact that most AMD tools (e.g. cram) use the same algorithm to normalize module ids. If somebody were to create a plugin that doesn't follow the standard AMD/CommonJS normalization, then we'd be causing breakage. Otherwise, it should Just Work™. :)

@gehan
Copy link
Author

gehan commented Nov 10, 2013

That was my way of saying I tried it and it didn't work ;)

I'll try again just in case!

@briancavalier
Copy link
Member

Make sure you try the latest dev branch. I made a few tweaks. If it doesn't work, post a test case and I'll iterate on it until it does :)

@gehan
Copy link
Author

gehan commented Nov 10, 2013

Will do! I was trying that specific commit before :)

@gehan
Copy link
Author

gehan commented Nov 10, 2013

Nope sorry! :(

Compiling wire/wire!app/viewModels/UserPane
cram failed:  Cannot find module '/home/vagrant/conversocial/frontend/static/app/viewModels/UserPane'
Error: Cannot find module '/home/vagrant/conversocial/frontend/static/app/viewModels/UserPane'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at loadScriptViaRequire (/home/vagrant/conversocial/frontend/node_modules/cram/amd_modules/curl/dist/curl-for-ssjs/curl.js:1572:4)
    at Object.priv.core.loadScript (/home/vagrant/conversocial/frontend/node_modules/cram/amd_modules/curl/dist/curl-for-ssjs/curl.js:1554:11)
    at Object.core.fetchResDef (/home/vagrant/conversocial/frontend/node_modules/cram/amd_modules/curl/dist/curl-for-ssjs/curl.js:949:9)
    at Object.core.fetchDep (/home/vagrant/conversocial/frontend/node_modules/cram/amd_modules/curl/dist/curl-for-ssjs/curl.js:1034:10)
    at getDep (/home/vagrant/conversocial/frontend/node_modules/cram/amd_modules/curl/dist/curl-for-ssjs/curl.js:904:21)
    at Object.core.getDeps (/home/vagrant/conversocial/frontend/node_modules/cram/amd_modules/curl/dist/curl-for-ssjs/curl.js:886:6)

@gehan
Copy link
Author

gehan commented Nov 10, 2013

Fails things like

thing:
    create:
        module: './viewModels/UserPane'

But also things like

thing:
    render:
        template:
            module: 'text!./views/user.html'

@gehan
Copy link
Author

gehan commented Nov 10, 2013

Compiling curl/plugin/text!./views/user.html
Compiling curl/plugin/css!./views/user.css
cram failed:  ENOENT, open '/home/vagrant/conversocial/frontend/static/lib/wire/wire!app/views/user.html'
Error: ENOENT, open '/home/vagrant/conversocial/frontend/static/lib/wire/wire!app/views/user.html'

@briancavalier
Copy link
Member

I def don't claim that it'll work with cram yet :) have you tried it without compiling?

@gehan
Copy link
Author

gehan commented Nov 10, 2013

Yep - already tried before and it works fine without cram! I said above actually :)

@unscriptable said it should work fine in cram though, so guess this is more directed at him!

@briancavalier
Copy link
Member

Oops, I missed your earlier comment about it working outside of cram, @gehan. Sorry about that!

I'll dig into the cram builder soon and see if I can figure out what's going on.

@briancavalier
Copy link
Member

Hey @gehan, relative plugin resource ids were just released in 0.10.3. We didn't verify the cram support yet, but I wanted to get this out now, and we'll follow with another release that will make sure #142 is all good.

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

3 participants