Skip to content
This repository has been archived by the owner on May 28, 2019. It is now read-only.

add build scripts to programmatically create files ace editor can consume #9

Closed

Conversation

chmontgomery
Copy link
Contributor

First, install the new tools by running npm install.

now, running gulp from gherkin3/javascript will generate a dist/gherkin/ folder containing all the files which can be dropped wholesale into the ace project under lib/ace/mode/gherkin/

This change depends on #7 and #8

@aslakhellesoy
Copy link
Contributor

I'm a little skeptical about adding a whole new build system just to accommodate for a different project.

I'd prefer to do this without gulp. I just added a browserify build and added some notes to the JavaScript README. Do you think what I suggest there would work?

@mattwynne
Copy link
Contributor

I think that, in general, we need a way for things like editor plugins to be separate projects. Otherwise this project will grow into the same size of beast that Gherkin 2 was.

@mattwynne
Copy link
Contributor

Oh my bad, I've read the code now. Ignore me.

@chmontgomery
Copy link
Contributor Author

Initially I tried browserify but when putting the result of browserify through ace's dryice build system it generates an invalid worker-gherkin.js. I'm not sure if this is a bug with dryice, with ace's implementation of it, or both. I've opened an issue for this problem under their repo: ajaxorg/ace#2433

In the meantime, the only way I was able to get valid ace files created was taking your original src files and wrapping them with amd before dropping them into ace's src. In the end, browserify would be cleaner so we don't have to do any hackey string replacements. Unfortunately, as of now, I haven't found a way to do it.

As far as a different build system, I'm not tied to gulp. However, it would be trivial to tie the gulp build into your make file the same way you did browserify. It would be as simple as:

dist/gherkin.js: .compared node_modules/tea-error/lib-cov/error.js
    mkdir -p dist
    ./node_modules/.bin/gulp

@aslakhellesoy
Copy link
Contributor

@chmontgomery can you check if 55d2419 works? The javascript/dist/gherkin.js file created by make should be sufficient.

aslakhellesoy added a commit that referenced this pull request Apr 2, 2015
@chmontgomery
Copy link
Contributor Author

@aslakhellesoy I just tried 55d2419 and the ace build still generates invalid worker-gherkin.js files as described here: ajaxorg/ace#2433. It seems to be a limitation of their build system... I also tried wrapping the file you generate with amd syntax in a couple of different ways with no success.

@chmontgomery chmontgomery force-pushed the build-for-ace-editor branch from dc2750b to fe474db Compare April 2, 2015 12:56
@aslakhellesoy
Copy link
Contributor

FYI - I have already wrapped the entry-point in AMD syntax: https://github.com/cucumber/gherkin3/blob/master/javascript/index.js

So the browserified javascript/dist/gherkin.js should already be AMD compatible - in theory. I haven't tested it. To narrow it down you could try to create a new example in javascript/examples that uses an AMD loader (require.js perhaps) to verify that it's indeed working correctly with AMD.

@chmontgomery chmontgomery force-pushed the build-for-ace-editor branch from fe474db to 38525ab Compare April 2, 2015 13:12
@chmontgomery
Copy link
Contributor Author

Most likely your generated file works in a stand alone amd example, the problem is with ace's build system. Dryice needs to be able to understand the file we're giving it so it can properly generate the different variations of the worker-gherkin.js file. I've tried every way I can think of to take your browserified file and pass it off to ace with no success. Unfortunately I don't have the time to work through ace's build file and/or dryice and provide a fix, hence this issue: ajaxorg/ace#2433

@chmontgomery chmontgomery force-pushed the build-for-ace-editor branch from 38525ab to e49aa8d Compare April 2, 2015 13:49
@aslakhellesoy
Copy link
Contributor

They are not making it easy!

Out of curiosity - wouldn't it be easier to keep a Gherkin plugin for Ace outside the Ace project? Doesn't Ace provide a simple mechanism to plug in 3rd party plugins?

Codemirror makes that super easy. I've created a 3rd party Codemirror plugin, and it lives its own life.

Shoehorning something into Ace sounds like an uphill struggle....

@chmontgomery
Copy link
Contributor Author

agreed, shoehorning ace is a losing battle... that's why I went with my original approach... taking your src files and wrapping them with a simple amd syntax. That seemed to play nice with ace.

I'm not an ace expert but they don't seem to have a plugin system. For other parsers they support like javascript, css, json, etc, they're checked into the ace repo directly under lib/ace/mode/<language>, just like your's would be.

The problem with supporting our own 3rd party library is we would still need to build ace compatible files. Ace generates build files for src, src-min, src-min-noconflict, and src-noconflict. Each of these versions are wrapped differently to work in various environments. We could copy ace's build system into a separate repo and modify it to our liking, but I'd prefer not to maintain something like that.

I've updated this PR to handle licensing properly so that the result, dist/gherkin/**, can be dropped whole-sale into the ace repo and run through their build system. I've added a new make target for this called ace

@chmontgomery chmontgomery force-pushed the build-for-ace-editor branch from e49aa8d to 9285afc Compare April 2, 2015 14:25
@@ -55,6 +55,9 @@ dist/gherkin.js: lib/gherkin/parser.js ../LICENSE node_modules/.fetched
echo '*/' >> $@
./node_modules/.bin/browserify index.js --ignore-missing >> $@

ace: lib/gherkin/parser.js ../LICENSE node_modules/.fetched
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the target should not be ace, but the path to the file that gulp produces. Otherwise, make won't be able to detect that there is nothing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. Will update

@chmontgomery
Copy link
Contributor Author

updated js Makefile to only build when needed

@aslakhellesoy
Copy link
Contributor

I appreciate your effort to bring Gherkin to Ace, but your approach is introducing extra work for both the Gherkin team and the Ace team:

I'll illustrate it with a dependency diagram:

    ┌────────┐  
    │        │  
    │        ▼  
┌───────┐  ┌───┐
│Gherkin│  │Ace│
└───────┘  └───┘
    ▲        │  
    │        │  
    └────────┘  

You have to remove the maintenance burden from the Gherkin and Ace teams by inverting the dependencies:

┌───────┐    ┌───────────┐    ┌───┐
│Gherkin│◀───│Gherkin-Ace│───▶│Ace│
└───────┘    └───────────┘    └───┘

You should create a separate GitHub repo where you put your gherkin_worker.js file, but you don't copy Gherkin source code into this repo. That's how JavaScript code was reused before package managers such as npm and bower became mainstream.

You'll have the freedom to use your favourite build tools, and you'll produce an Ace plugin. I did a quick search in the Ace docs for how to write plugins, and didn't find anything. That doesn't mean Ace doesn't have a plugin API - it just means you'll have to ask the Ace team for help.

When you distribute your plugin on npm and/or bower, I recommend you don't embed the Gherkin code there either, just declare a dependency in package.json/bower.json. Consumers of your plugin will then get Gherkin as a transitive dependency.

What do you think about this approach?

@chmontgomery
Copy link
Contributor Author

I think that makes a lot of sense. I originally wanted to do something similar but, as we mentioned earlier, fighting ace would be an uphill battle. I was just trying to fit into their system without too much friction.

We can close this PR and I'll pull the couple build scripts into my own repo where it can be published separately.

@chmontgomery
Copy link
Contributor Author

@aslakhellesoy can you publish gherkin3 to npm? right now all that's in npm is version 2.

@aslakhellesoy
Copy link
Contributor

Gherkin3 isn't stable yet, so publishing now would be premature. I don't expect significant changes, but I'm sure there will be some minor ones. We don't really know until we start replacing Gherkin2 with Gherkin3 in the various Cucumber implementations. That will start happening as soon as #12 is resolved.

Until Gherkin3 is released to npm you can depend on the git repo:
https://docs.npmjs.com/files/package.json#git-urls-as-dependencies

I'm glad we're making progress on this!

@chmontgomery
Copy link
Contributor Author

@aslakhellesoy I can't depend on this project from my package.json because this project's package.json is not at the top level of the project. Currently npm does not support depending on a package in a subdirectory of a repo: npm/npm#2974

@aslakhellesoy
Copy link
Contributor

Oh I see. There is a similar problem for PHP as described in #4. We might need to split Gherkin up in several git repos.

@aslakhellesoy
Copy link
Contributor

@chmontgomery Now there is a standalone repo:

https://github.com/cucumber/gherkin-javascript

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