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

Add 'X-Requested-With' header to liftVanilla #1746

Closed
wants to merge 9 commits into from
Closed

Conversation

eltimn
Copy link
Member

@eltimn eltimn commented Dec 15, 2015

fixes #1745

I added a way to run JavaScript tests, but they need to be run in a browser manually by loading web/webkit/SpecRunner.html

Here's the output:

screenshot from 2015-12-15 15 42 11

@eltimn
Copy link
Member Author

eltimn commented Dec 16, 2015

I went ahead and added an npm script to run the tests and jshint since we're using travis. You can still run the tests manually with a browser so you don't need to install node to run them.

@Shadowfiend @farmdawgnation is there a specific reason there's no language: scala entry in travis.yml? Without it travis assumes it's a ruby environment and installs ruby. It doesn't seem to hurt anything, but seems wasteful.

@Shadowfiend
Copy link
Member

Re: node, is there any reason we can't just use Nashorn given that we've imposed build-using-Java-8-minimum semantics? I realize we're currently building with Java 7 until we release M7, but it seems worth it to keep the build depending solely on Java and Java package resolution (via webjars?).

successMsg(evt.success);
}
else if (evt.failure != null) {
else if (evt.failure !== null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We sure these three don't break anything? undefined != null is false while undefined !== null is true…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I'll revert it. I changed a couple of other ones that I'll revert as well.

@Shadowfiend
Copy link
Member

Also, using webjars would probably allow us to not import jasmine directly into the repo, etc etc.

Oh, and re: language in the travis file: I don't think there's a reason, but not sure.

@eltimn
Copy link
Member Author

eltimn commented Dec 17, 2015

Part of the difficultly with testing is the way we set lift, liftJQuery, and liftVanilla as global variables. That's why I went the browser and phantomjs route. I couldn't figure out a way to get the file loaded to be available for the tests.

What I'd like to do is rewrite it using ES6 modules and transpile them into ES5 and requirejs modules. That would make it easier to do testing.

While we could use Nashorn directly, it's probably easier to just use sbt-web and plugins. We'd need that for the webjars anyway.

So, my thinking was with the node stuff at least we have some way to write tests for the JavaScript code.

I could add sbt-web to get the dependencies, but without rewriting the js code, I don't see any other way to have tests for it.

@Shadowfiend
Copy link
Member

Hmmm… I see. That definitely sounds like a bigger hunk of work, and I think this is progress, so I'm game!

@eltimn
Copy link
Member Author

eltimn commented Dec 19, 2015

Rhino in yuicompressor conflicts with the Rhino version in sbt-web. So running any sbt-web tasks doesn't work. It's installed only to download the webjar dependencies.

I tried to include yuicompressor intransitively in sbt-yiucompressor and use a newer version of rhino, but it wouldn't compile that way.

It would be nice if we could get rid of all of those old JavaScript libraries we have in toserve.

I decided to leave the jshint comments in the js source as that's really the best way. I did change the eval ones to a single line, but had to add some for the != null ones.

If this looks good, I'll squash my commits and issue a new pull request.

@@ -145,4 +147,4 @@ changes to Lift. We do, however, accept some small changes and bugfixes into Lif

## Continuous Integration

SNAPSHOTs are built at CloudBees: https://lift.ci.cloudbees.com/
SNAPSHOTs are built by [Travis CI](https://travis-ci.org/lift/framework)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah good catch!

@@ -680,7 +685,7 @@
success: onSuccess,
error: function(_, status) {
if (status != 'abort')
return onFailure.apply(this, arguments)
return onFailure.apply(this, arguments);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codacy flagged this as being better with curlies around the if body, I tend to agree.

@Shadowfiend
Copy link
Member

Yeah, this is looking good. I'm down for a squash+merge when you're comfortable with it :)

@eltimn
Copy link
Member Author

eltimn commented Dec 26, 2015

New PR at #1748

@eltimn eltimn closed this Dec 26, 2015
@farmdawgnation farmdawgnation deleted the tcn_issue_1745 branch September 2, 2024 13:44
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

Successfully merging this pull request may close these issues.

Add 'X-Requested-With' header to liftVanilla
2 participants