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

rhino version not up to date #1405

Closed
obecker opened this issue Jul 6, 2013 · 12 comments
Closed

rhino version not up to date #1405

obecker opened this issue Jul 6, 2013 · 12 comments

Comments

@obecker
Copy link
Contributor

obecker commented Jul 6, 2013

Apparently the less-rhino version is out of date.
less-rhino-1.4.0.js (in the dist directory) is 6 months old and doesn't contain (for example) the new functions pi, hsvsaturation, convert etc.
For version 1.4.1 there is no less-rhino file at all.

I have written a gradle plugin for less.js (based on less-rhino) and I would like to update it to the latest version.

@lukeapage
Copy link
Member

Someone needs to test it and fix integration issues then we can release.. I
haven't got round to it and there is no rhino test suite.

@CirrusABS
Copy link

I think this is related to the issue i entered. #1496. sorry for the double entering of the same issue did not find this one until after i submitted mine and did some more goggling

@obecker
Copy link
Contributor Author

obecker commented Oct 27, 2013

I have forked less.js and created a gradle build script that runs some of the less tests (is is not complete yet, but it is a start - also be aware that running gradle, which invokes launching a JVM, is much much slower than running node ...).
Furthermore I have rewritten major parts of rhino.js to get the tests running.

This is the repo: https://github.com/obecker/less.js/tree/rhino

To run the tests use

grunt concat:rhino
gradlew testRhino

I would like to hear about your opinion and how we should continue with fixing the rhino integration.

@SomMeri
Copy link
Member

SomMeri commented Dec 3, 2013

@lukeapage @jonschlinkert I do not want to promise anything soon, but lets say I would want to have a look on less.js and rhino. Obviously, tests for rhino integration require java installed and rhino jar.

  • Do you think it is a good idea to run all that from grunt? Not everyone who may want to compile less.js need java version.
  • Do you think it is a good idea to include Rhino jar into this repo? If the answer is no, is there some other place where I could put all that?

The ideal would be if browser, node.js and rhino versions would run the same tests and tests would not require changes on three places when something changes. Only if you agree with that plan of course.

@obecker
Copy link
Contributor Author

obecker commented Dec 3, 2013

@SomMeri as I said, you might want to have a look at my changes https://github.com/obecker/less.js/compare/rhino

Currently the test cases for

  • test/less
  • test/less/compression
  • test/less/errors
  • test/less/legacy
  • test/less/static-urls

pass without failures.

I haven't got time yet to figure out what to do for

  • test/less/debug
  • test/less/no-js-errors
  • test/less/sourcemaps

The test sources (i.e. the less files and the expected css file) are (and should) be shared for node and rhino. I don't think we need special rhino tests. However, the execution of the tests with rhino has been re-implemented using gradle. This would mean every change in the test setup (including test parameters) must be transferred to the gradle script. If someone finds a way to execute all this from grunt, this is more than welcome. However, for the time being I've written the test script using gradle, so missing tests are no show stopper anymore.

Concerning the rhino jar: gradle provides dependency management mechanisms (as already known from Maven or Ivy), so there's no need to include it in this repository. All that is needed is an installed JVM and then the gradlew script will download automatically all required Java libraries (including rhino).

@obecker
Copy link
Contributor Author

obecker commented Dec 3, 2013

BTW: the author of https://github.com/asual/lesscss-engine managed to use the browser less-x.x.x.js along with additional scripts for rhino. Maybe that's the way to go.

@SomMeri
Copy link
Member

SomMeri commented Dec 3, 2013

@obecker I looked at it, but I did not know much about gradle. How do I run your tests? I will try again. The gradlew script downloading rhino is a very good news. However, graddle then must be stored somewhere?

I think it would be best if also less.js configuration (options) would be shared for all three versions. Adding new configuration option would then require a change on only one place. Right now it requires a change on two places (node and browser) and there is no rhino test.

lesscss-engine requires env.js, while running rhino version is straightforward 'java -jar js.jar test.less' - so it is simpler.

@SomMeri
Copy link
Member

SomMeri commented Dec 7, 2013

@obecker Ignore my previous comment, I got it now. You did a lot of work there. gradlew.bat testRhinoErrors fails on my setup, the rest is passing.

@lukeapage
Copy link
Member

Long term, for 2.0 (and this is the next thing I want to do) I want to have
less interface with its environment and assume only es5 functions.

Then the browser and node interfaces will be cleaner, I can write a .net
version using v8 and a proxy object and rhino should also be easier.

There is no reason we don't have one interface for browser vs file
importing and the file one can work via interface in rhino and node.. maybe
browser even doesn't need to be different.

I would then probably put the rhino version in a dependent repository
organised however that makes sense.

I don't see an issue with requiring node for rhino developent but I'm not
the one doing it (since I fixed it in 1.3.4) so I don't mind if it doesn't
use grunt.

Anything that doesn't go against the above plan I would love to get in.

@SomMeri
Copy link
Member

SomMeri commented Dec 12, 2013

Alright, I forked less.js repo and pulled all @obecker changes into it. That repo is now cotains both latest less.js and obecker changes.

Once it has zero open issues, we can decide whether we will push code changes into less.js right now or only after refactoring @lukeapage mentioned.

@SomMeri
Copy link
Member

SomMeri commented Jan 15, 2014

@dhaber @lukeapage Rhino build and tests are ready for review. Pull request: #1806.

lukeapage added a commit that referenced this issue Jan 21, 2014
rhino version not up to date (#1405)
@SomMeri
Copy link
Member

SomMeri commented Jan 21, 2014

Since #1806 has been merged in, I'm closing this.

@SomMeri SomMeri closed this as completed Jan 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants