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

Drop najax global by default? #246

Closed
rwjblue opened this issue Oct 31, 2019 · 5 comments · Fixed by #247
Closed

Drop najax global by default? #246

rwjblue opened this issue Oct 31, 2019 · 5 comments · Fixed by #247
Labels
Milestone

Comments

@rwjblue
Copy link
Member

rwjblue commented Oct 31, 2019

As we continue work towards the next major version (will be v3.0.0), I'd like to seriously consider dropping najax from the default set of global properties added by this library.

The primary reasons are:

  • Recent versions of ember-data (3.12+) will automatically use ember-fetch (or global fetch) if present, and does not need special logic around najax
  • New applications (as of ember-cli@3.13.0) include ember-fetch by default, and do not include jQuery by default. Considering that najax is a jQuery.ajax emulation API, I think this library should avoid exposing it.
  • najax (as a dependency of this library) is difficult for the host application to control (e.g. get their own version), and would be better if they provided it themselves (via buildSandboxGlobals API)
  • Providing backwards compatibility is very easy (add najax: require('najax') via buildSandboxGlobals)
  • It seems better to "follow a spec" (suggesting fetch usage)

Thoughts?

@rwjblue rwjblue added this to the v3.0.0 milestone Oct 31, 2019
@rwjblue
Copy link
Member Author

rwjblue commented Oct 31, 2019

In case it wasn't clear from the bulleted list, I'm strongly in favor of removing 😸

@ryanto
Copy link
Contributor

ryanto commented Oct 31, 2019

👍 Sounds good.

najax is something that you eventually run into and have to learn when doing FastBoot work, it's not bad, but it is another thing you have to know.

Using fetch sounds like a great idea. I think that both fetch and node-fetch are pretty common tools in SSR and do a good job at providing API consistency. The fact that this all comes bundled up for us with ED + ember-fetch is awesome, so strongly in favor of this.

@sdhull
Copy link

sdhull commented Oct 31, 2019

Frankly I only vaguely remember the differences between these libraries. IIRC the primary difference I ran into was something related to ajaxOptions/beforeSend? Like fetch doesn't have ajaxOptions maybe? I did find this which may be related. I believe it's pretty easy to refactor beforeSend by overriding ajax() method itself and calling super.

I think if it truly is as easy as najax: require('najax') to fix apps that break as a result of this change (and that is well-documented), seems fine to me.

@rwjblue
Copy link
Member Author

rwjblue commented Oct 31, 2019

@sdhull

For a long time ember-data itself only supported making jQuery requests, and since najax provides a nice "jQuery like" API it was a very convenient way to be able to move forward with SSR in general without having to rewrite all of the request logic in ember-data. Now that recent versions of ember-data will use fetch out of the box, the najax dependency / global is just hanging around for no good reason.

I think if it truly is as easy as najax: require('najax') to fix apps that break as a result of this change (and that is well-documented), seems fine to me.

👍 awesome, thank you! I'll make sure it is well documented (in the release notes, in the README, and likely in other projects as we update them to fastboot@3).

@tomdale
Copy link
Contributor

tomdale commented Oct 31, 2019

I think this has served it's purpose. 👍 on dropping.

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

Successfully merging a pull request may close this issue.

4 participants