Skip to content
This repository has been archived by the owner on Jan 30, 2023. It is now read-only.

Basic fastboot support #75

Merged
merged 1 commit into from
Apr 12, 2016
Merged

Conversation

topaxi
Copy link
Contributor

@topaxi topaxi commented Mar 11, 2016

This adds basic fastboot support by using the provided URL module by ember-fastboot-server and najax.

I get warnings as najax does not implement jqXHR.getAllResponseHeaders(), but this should work for basic ajax requests.
It seems to be implemented in najax on the master (najaxjs/najax#26) branch, I'm not sure if this is released in a npm package yet.

@alexlafroscia
Copy link
Collaborator

Would we need to include najax as a dependency of ember-ajax for this to work? Or would it be included some other way?

@topaxi
Copy link
Contributor Author

topaxi commented Mar 11, 2016

najax is provided to the sandbox by ember-fastboot-server: ember-fastboot-server/lib/ember-app.js#L32

@alexlafroscia
Copy link
Collaborator

Awesome, that's good to know. My one concern is accepting this without automated testing in Fastboot... What do you think @taras?

@topaxi
Copy link
Contributor Author

topaxi commented Mar 11, 2016

Yeah, tests would be great, I don't know the current state on fastboot testing.

Related discussions: ember-fastboot/ember-cli-fastboot#111 and ember-fastboot/ember-cli-fastboot#68

@alexlafroscia
Copy link
Collaborator

Thanks for linking those, I was just reading them myself. Maybe we should pull it in, add real tests once it's supported, and update the README to say that Fastboot is supported... Unofficially?

@alexlafroscia
Copy link
Collaborator

I made a little example app, and I think that the lack of support for getAllResponseHeaders is going to be a deal breaker for now. I tried making a little example app that just hits Github's API and grabs the list of issue for this repo, and immediately got an error due to najax.getAllResponseHeaders not being implemented. The same works fine with a regular ember serve.

@alexlafroscia
Copy link
Collaborator

Also, one other thing I would like to do: if we can't import NAJAX as a module, could we expose our own module that exports najax and consume it within the addon that way? Either that, or add najax to the globals list in the .jshintrc, I don't love declaring it at the top of the file.

@topaxi
Copy link
Contributor Author

topaxi commented Mar 14, 2016

getAllResponseHeaders is implemented in the latest najax release and also in the latest ember-fastboot-server (though the latest ember-fastboot-server isn't released yet`.

It works for me as long as I don't need any headers (the getAllResponseHeaders error is just a warning in my case), when the next ember-cli-fastboot is released, this should work too.

I'll look into creating an ajax module which exports either najax or jQuery.ajax.

@alexlafroscia
Copy link
Collaborator

I'll look into creating an ajax module which exports either najax or jQuery.ajax.

That sounds perfect

getAllResponseHeaders is implemented in the latest najax release and also in the latest ember-fastboot-server (though the latest ember-fastboot-server isn't released yet`.

I see, I'll look see if I can get it working with the current master then.

@alexlafroscia
Copy link
Collaborator

I think the issue that I'm seeing is that the najax version that comes bundled with Fastboot right now fails when making the request that I'm trying to make. I tried it using the latest release of najax, and the request seems to work fine.

Assuming that this works correctly (and as far as I can tell, it does), I'm in favor of pulling this in and gaining basic Fastboot support once those few changes are made regarding importing ajax from a separate module.

@alexlafroscia
Copy link
Collaborator

Huh, I keep getting the following error with najax:

Ember Data Request GET https://api.github.com/repos/ember-cli/ember-ajax/issues?state=open returned a 0\nPayload (Empty Content-Type)\n"SyntaxError: Unexpected token R\n at Object.parse (native)\n at IncomingMessage. (/Users/alex/Projects/open-source/tmp/fastboot-test/node_modules/ember-cli-fastboot/node_modules/ember-fastboot-server/node_modules/najax/lib/najax.js:135:23)\n at emitNone (events.js:85:20)\n at IncomingMessage.emit (events.js:179:7)\n at endReadableNT (_stream_readable.js:906:12)\n at nextTickCallbackWith2Args (node.js:474:9)\n at process._tickCallback (node.js:388:17)"

Doesn't seem to happen using either najax@0.4.0 or 0.3.2 when using najax by itself in Tonic, it only happens in Fastboot. Request works fine in the browser, too.

@alexlafroscia alexlafroscia added this to the 2.1 milestone Mar 17, 2016
@topaxi
Copy link
Contributor Author

topaxi commented Mar 29, 2016

Rebased and moved the ajax function into a module in utils. I also moved is-fastboot into utils as I think it fits better in there.

@tchak
Copy link

tchak commented Apr 2, 2016

I started some work on using ember-network integration. This will give us a first class fastboot support. See #81.

@alexlafroscia
Copy link
Collaborator

Sorry I haven't gotten to check on this yet. Been traveling all weekend, I'll get to this today.

Checked it out, this seems 👍 to me, despite the lack of testing in the Fastboot environment (not your fault! Tooling just isn't there yet). @taras and I are just figuring out when we want to pull this in relative to the 2.0 release, so hang tight.

@alexlafroscia
Copy link
Collaborator

Okay, so now that 2.0 has landed, I think that we should pull this into master so that people that want this have a way of getting it. Think we're ready @taras?

@alexlafroscia alexlafroscia modified the milestones: 2.1, 3.0 Apr 9, 2016
@alexlafroscia alexlafroscia merged commit 35a498f into ember-cli:master Apr 12, 2016
@alexlafroscia alexlafroscia mentioned this pull request May 29, 2016
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants