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

Feature - Nashorn support (Java 8) #2985

Closed
wants to merge 1 commit into from

Conversation

artfiedler
Copy link

This should resolve multiple open issues [Issue #2063] [Issue #2316]

  • Implemented lib/less-nashorn
  • Added support for test cases to run with nashorn
  • Implemented bin/lessjjsc

Requirements: jdk8 (I tested with jdk1.8.0_102)

I created lessjjsc instead of modifying lessc because I was not sure how to do a fallback Shebang if (!node) then jjs. I also wasn't having luck figuring out how to make jjs include -- in the shebang. Do note that nodejs is much faster than nashorn but it should be useful regardless.

Ways to run from bash (tested on windows)

  1. less.js-2.7.1> bin/lessjjsc -- -verbose benchmark/benchmark.less
  2. less.js-2.7.1> bin/lessjjsc -- -<<EOF
    > .class {
    > width: 17pt + 5px;
    > }
    > EOF
  3. less.js-2.7.1> bin/lessjjsc -- -verbose - < benchmark/benchmark.less
  4. less.js-2.7.1> bin/lessjjsc -- -
    >> Type .less input, when done end with Ctrl-D
    > .class {
    > width: 20px + 7pt;
    > }^D

To run on windows/linux without bash or without shebang

Commands are the same just prefix with jjs

  1. less-2.7.1> jjs bin/lessjjsc -- benchmark/benchmark.less output.css

This should run on any system that has jdk1.8 no additional libs or requirements.

Testing

I got the majority of the test cases working, I don't have grunt, browserfy, and 993285409238409238 other things and I've never used them so this is how I ran my tests...
less.js-2.7.1> jjs test\index.js
> (snip)
> 6 Failed, 311 passed

Notes

The 6 that fail are related to SourceMap's I did not implement that in the environment.js so it throws and error instead.

I also was testing @import (inline) "https://lesscss.org/public/css/index.css"; and that works but there are a few // TODO:'s around for instance in the url-manager I put together joining base + relative url's but wasn't quite sure how to test. Also diff(left,right) there is a TODO for in case the line count doesn't match for the test cases for now everything seems to match so that works.

I choose to base the modifications off of the 2.7.1 tag because that's the production version listed on the website... I should be able to update for 3.x I have not looked into that at all though.

Added support for test cases to run with nashorn

Implemented lessjjsc
@matthew-dean
Copy link
Member

Is this a replacement for less-rhino?

@artfiedler
Copy link
Author

Yes, as less-rhino doesn't work... I made less-nashorn that can utilize java's integrated javascript engine (nashorn)

@artfiedler
Copy link
Author

I also have a wrapper for lesscss to use directly in java but I never got around to publishing that along with my other wrappers.

@matthew-dean
Copy link
Member

matthew-dean commented Apr 4, 2017

Okay, perfect. One of my concerns is that less-rhino got stale for a number of reasons. Are those reasons addressed in these ways:

  1. Integration into the main build / test process (builds by default?)
  2. Successfully running all (relevant) tests through this environment.
  3. Proper abstraction between environments. Meaning: A lot of less-rhino was copy/pasted from lessc and less-node. When I updated and made some breaking changes for 3.x, I didn't really know what was relevant to update to less-rhino because I don't know that environment. If a code is relevant to two pieces, it should ideally be moved to an abstracted file (like abstract-file-manager.js), or integrated into less core (/lib/less), unless it's completely irrelevant to less-browser, in which case it can be abstracted and imported into less-node and less-nashorn. For example: you mention you made lessjjsc-helper.js because you weren't sure about modifying lessc. A better approach is to have both lessc and lessjjsc-helper.js import relevant code such as the printouts for options.
  4. Documentation for maintainers who may not know Nashorn into how it differs from a normal JavaScript environment and how to maintain that integration into Less. This may include, if pieces are abstracted, documentation of those pieces in Less core. And explaining JDK on the docs: http://lesscss.org/3.x/usage/#developing-less

I know that's a lot of work, but it would help make sure that the work you just did stays integrated into the Less repo for a long time.

@artfiedler
Copy link
Author

  1. The lessjjs uses require() and contains some code to make its own require function as nashorn doesn't support nodes require. I assume to integrate it would require very little work, I did not have lesscss's build setup as I'm not familiar with it off the top of my head.
  2. All the relevant tests pass and should pass when integrated with the environment. I believe after I posted this code I fixed the 6 failed source map as I didn't realize at the time it was as simple as loading the Mozilla source map lib... in my post here you can see how to run the tests "less.js-2.7.1> jjs test\index.js"
  3. The environment is separated excluding the fact that lessjjsc is a modified copy of lessc... should be a very simple compare to update with a modified lessc. As I believe I noted previously in my post the reason I didn't simply merge lessc and lessjjsc was simply because the shebang's are different and I didn't know a method of making that dynamic. The remaining script after the shebang could technically be merged into a common script... I didn't do that at the time because I was unsure of what is preferred by lesscss authors. I separated so anyone could easily compare to see the changes made and then make a decision.
  4. The only documentation that is made is what is mentioned in this feature request, and any additional comments made on the issue thread.

@artfiedler
Copy link
Author

I'll also note that the less-nashorn section is modeled after the less-node section, but it's implementation is not a copy of node, but instead implemented like node.

@matthew-dean
Copy link
Member

@seven-phases-max Thoughts about merging?

@seven-phases-max
Copy link
Member

seven-phases-max commented Apr 4, 2017

I don't know - I'm so totally not connected to Java and Java-specific Less impl. so I barely can't have any opinion (I guess we could simply rely on @artfiedler's opinion - if he thinks it's fine to put all that efforts and make a PR then I guess it's fine).

@matthew-dean
Copy link
Member

@seven-phases-max What about merging vs a separate less/less-nashorn repo?

@seven-phases-max
Copy link
Member

Well, a separate repo is of course preferred (considering that ideally and potentially it has its own specific test environment, issues and finally a maintainer(s)).

@artfiedler
Copy link
Author

A separate repo sounds like total overkill, the core of less is the same, and all the tests. To make them all one the lessc and lessjjsc could be abstracted into a LessCompiler, and the lessjjsc differences could be provided in a JJSLessCompiler implemention file. There still would be two bin files however, My guess is the lessc version would be two lines of code (shebang and require the lesscompiler) and the lessjjsc would have just an additional require function ontop of that with different shebang and require JJSLessCompiler.

Think of "jjs" as "nodejs" it executes javascript, this isn't a java implementation but a nashorn (java's javascript engine) implementation.

@seven-phases-max
Copy link
Member

seven-phases-max commented Apr 5, 2017

Well, a separate repo does not necessary mean a complete and totally independent fork (as in "copy-paste" everything)... A repo may have just less-java dir and share the rest of code with the main repository (e.g. via submodules). Or in opposite (i.e. a less/less-java being a submodule of less/less.js) The main benefit is that we could simply give it away to the people interested (e.g. you) so that they could just make it go w/o need to continuously consult/wait/ask/etc for ignorants (e.g. me). (Technically there were the same plans for other implementations - specifically less-browser).

@matthew-dean
Copy link
Member

@artfiedler What @seven-phases-max said. You could also add Less.js as a dependency (or peer dependency). As a separate repo, you could be added as the maintainer of that repo (it could still even live under the Less org).

It would also allow issues to be environment specific, which would be more manageable.

We could alternatively make a less-nashorn repo and make it a submodule of Less. That would keep it structurally identical to the current environment stuff, but give environments their own commit histories (and issues, Github maintainers, etc).

How do other projects manage these "environment build" scenarios?

@matthew-dean
Copy link
Member

matthew-dean commented Oct 10, 2017

This needs a decision. I think the lean though is toward something in the less org but less-nashorn. (or less-java?).

@matthew-dean
Copy link
Member

@artfiedler Please set up a working less-java repo and I think we'd be happy to add it under the Less org here with you as the principle maintainer for that repo.

@wendal
Copy link

wendal commented Dec 8, 2017

@artfiedler Great Job!

@zozoh
Copy link

zozoh commented Dec 8, 2017

@artfiedler Awesome, it is just what we need!

@cmsimike
Copy link

I am looking forward to this PR coming through

@wendal
Copy link

wendal commented Dec 20, 2017

we had create a repo lessc4j, using codes from this pull request, so we can use Java API to compile less files.

https://github.com/nutzam/lessc4j

@azine
Copy link

azine commented Jan 19, 2018

@artfiedler good job! I hope you'll find a way to get this integrated into the "less"-repos.

@stale
Copy link

stale bot commented May 19, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 19, 2018
@zozoh
Copy link

zozoh commented May 24, 2018

@wendal good job!

@matthew-dean
Copy link
Member

Closing this since there's no activity and no apparent interest in setting up a separate repo. There are also Less Java ports here -> https://github.com/LucasBassetti/awesome-less#java. We really don't want to get into a situation like the less-rhino port in this repo with no one available to maintain it.

So, if there's a active maintainer of a Java-wrapped Less repo, then we could perhaps move it under the Less org here on Github and assign that person. Either way, supporting browser/Node is complicated enough in this repo.

@matthew-dean
Copy link
Member

It seems like someone could take @wendal's repo, use something like https://github.com/eirslett/frontend-maven-plugin to use Node.js/NPM to download the latest Less version via Maven (so that it's not copied into the repo), and then it would keep synced up with the main Less repo, if you had a proper package.json file. @artfiedler ?

@artfiedler
Copy link
Author

Point maybe mute as nashorn is already being deprecated in java. I had wrote a pure java version as well but never got around to posting.

The way I coded it I had hoped you guys could just modify your build script to generate the jjs version as it shouldn't really need maintenance as it just ran on a simulated nodejs environment inside java, less js's base code wasn't modified as I remember.

@matthew-dean
Copy link
Member

matthew-dean commented Jul 9, 2018

@artfiedler Hmm okay, so you agree this shouldn't be re-opened?

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

Successfully merging this pull request may close these issues.

7 participants