update ace to v1.2.9; fix ts build; #93

filipsch · 2017-07-20T16:52:51Z

update aceTag to v1.2.8
updated build/update-ts.js to follow new structure of DefinitivelyTyped repo.

thlorenz · 2017-07-20T22:19:46Z

Thanks @filipsch.
Could I get some more collaborators to review this? On quick skim LGTM.

gokmen

it LGTM in general. It's not related with this PR but there is a minor that I commented on.

gokmen · 2017-07-20T22:29:23Z

ext/searchbox.js

        this.editor = editor;
    };
+
+    this.setSession = function(e) {
+        debugger


that seems weird, it's introduced with https://github.com/ajaxorg/ace/pull/3329/files#diff-2b1fc8367171d51dd3148748a50accb2R88 on Ace repo 23 days ago. maybe we should let them know first fix there and merge after the change.

created a PR for that ajaxorg/ace#3358 fyi.

OK let's wait for that to go through and then run the upgrade again.
Don't want to pull anything in that introduces an obvious bug from the upstream code base.

it's merged on upstream, if @filipsch can update the code we are good to go I believe 👍

I guess I'll have to wait for ajaxorg to make a new release of ace, right?

Yeah better since we don't want to depend on an unreleased version.

…hub.com/sevin7676/Ace.Tern/tree/master/ace-builds/src-noconflict to be available for brace "build/update" proces - hack update script to include ext-tern.js and worker-tern.js to the build - include update script fix from thlorenz#93

pankitgami · 2017-08-22T06:57:29Z

When will you merge this request? As ace is on 1.2.8 and this package is on 1.2.6. And there are lots of changes between both the release in terms of worker, theme, mode etc.

filipsch · 2017-08-22T11:13:32Z

@gokmen noticed that the people from ace merged in a bug (a debugger call) and this bug is in 1.2.8 of ace. A fix of this has been merged, but we're still waiting for the people of ace to bump their version, so we can have brace depend on a new release (without the bug).

pankitgami · 2017-08-25T09:55:01Z

This version of the brace has a bug in php-worker.

editor.js:3609 Could not load worker TypeError: Cannot read property 'split' of undefined

shepmaster · 2017-08-26T03:24:58Z

Is there a link to an issue requesting a new ace release that someone could provide?

FWIW, I'd be OK with using Ace 1.2.8 even with the debugger statement, especially considering there are bugs that impact my downstream users fixed in 1.2.8.

To my knowledge, the debugger statement will only have an effect when the user has the browser inspector open, and in that case you presumably understand what you are doing and how to continue past a rogue debugger.

From another POV, if brace upgrades to 1.2.8, then more people can experience any issue that this causes and then check in with upstream to get it fixed / released.

shepmaster · 2017-10-17T13:47:06Z

Ace 1.2.9 has been released! Now with one less debugger statement 😺

filipsch · 2017-10-17T13:48:53Z

@shepmaster I'll update the PR then to depend on v1.2.9, and see if the builds still work.

filipsch · 2017-10-17T14:01:43Z

Done, but my new commit and push to the pr branch of datacamp/brace fork is not coming through for some reason.

shepmaster · 2017-10-17T14:12:42Z

Huh; I see the same behavior. Maybe it's just some GitHub slowness?

Edit — Yeah "We are investigating reports of elevated error rates." 11 minutes ago

Edit2 — I see it now! 🎉

filipsch · 2017-10-17T14:29:50Z

@shepmaster Okay cool; we're ready then. @gokmen can this be merged in now, and can a new release be made at the same time? Thanks!

gokmen · 2017-10-17T17:02:51Z

it's good to go for me 👍, on you @thlorenz

filipsch · 2017-10-21T18:23:05Z

@thlorenz ping!

thlorenz · 2017-10-27T14:18:15Z

Sorry, just seeing this now. LGTM

@filipsch made you collaborator.

Please merge to master following this guide.
I will then version and publish to npm.

Thanks.

Obviously @gokmen if you see this first and want to merge, fine by me.

Just ping me when done and I'll publish a new minor to npm.

filipsch · 2017-10-30T12:45:19Z

@thlorenz done! Ping me when new version is released and published; thanks!

thlorenz · 2017-10-30T13:27:31Z

@filipsch getting test errors (did you run npm test and open devtools?).
I don't see any error annotations in the editors, seems like something not working with the worker scripts.

I have 10/10 tests failing.

If we cannot solve this today we need to revert the commits from master.
I thought we had checked the tests before merging.

thlorenz · 2017-10-30T13:29:18Z

OK, confirmed that before these commits everything works fine.

> git reset --hard a417c2e
HEAD is now at a417c2e Fix link to DefinitelyTyped definition in README

> npm ts

filipsch · 2017-10-30T13:30:31Z

@thlorenz will look into it before EOD.

thlorenz · 2017-10-30T13:31:44Z

OK I force pushed to master, reverting the commits (hoping no one else but the two of us updated from master at this point).
The errors are a regression we cannot have, one main point of brace is that the worker scripts are inlined and error annotations just work out of the box.

Sorry to do this, but I feel this is better to have reverted these and now we can calmly figure out why things broke.

filipsch · 2017-10-30T13:32:16Z

@thlorenz completely understand; I should have run the test scripts before; my bad.

thlorenz · 2017-10-30T13:32:56Z

No problem (we caught it in time). Probably best to open another PR with the changes as I cannot reopen this one.

shepmaster · 2017-10-30T15:16:57Z

I force pushed to master, reverting the commits

For the future, you can just revert the merge. GitHub should even have a button:

thlorenz · 2017-10-30T16:14:23Z

@shepmaster I know you can do that, but that adds extra commits.
The fact that this was up for only 30 mins made me think no one would get hurt if I just force push ;)

LeviRosol · 2017-11-09T18:55:27Z

Is this still going to happen? I was looking forward to the update.

filipsch · 2017-11-09T22:45:38Z

@LeviRosol it's still on my todo list, but I'm pretty swamped at my daytime job currently. When doing this PR, I didn't dive into the brace codebase itself, and basically just updated the build scripts. Therefore, I guess it will take a pretty long time for me to figure out where and why the tests now fail. I have no idea how long this is going to take.

LeviRosol · 2017-11-10T06:29:59Z

no worries. I assumed as much. If I can find some time I'll try to lend a hand.

update ace to v1.2.8; fix ts build;

de1ed71

gokmen approved these changes Jul 20, 2017

View reviewed changes

shepmaster mentioned this pull request Aug 26, 2017

Auto indentation and adding of closing braces rust-lang/rust-playground#202

Closed

mikeyhew mentioned this pull request Oct 7, 2017

Ext/Searchbox: remove forgotten debugger ajaxorg/ace#3358

Merged

bump ace version to 1.2.9

3b92e3d

filipsch changed the title ~~update ace to v1.2.8; fix ts build;~~ update ace to v1.2.9; fix ts build; Oct 17, 2017

filipsch merged commit 651f084 into thlorenz:master Oct 30, 2017

thlorenz mentioned this pull request Oct 30, 2017

Fixes update script and run the most recent version #89

Closed

thlorenz mentioned this pull request Oct 30, 2017

Update brace with latest ace version #99

Closed

nightwing mentioned this pull request Nov 10, 2017

Update worker patch to work with ace 1.9 #109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update ace to v1.2.9; fix ts build; #93

update ace to v1.2.9; fix ts build; #93

filipsch commented Jul 20, 2017

thlorenz commented Jul 20, 2017 •

edited

Loading

gokmen left a comment

gokmen Jul 20, 2017 •

edited

Loading

gokmen Jul 20, 2017

thlorenz Jul 21, 2017

gokmen Jul 21, 2017

filipsch Jul 22, 2017

thlorenz Jul 23, 2017

pankitgami commented Aug 22, 2017 •

edited

Loading

filipsch commented Aug 22, 2017 •

edited

Loading

pankitgami commented Aug 25, 2017

shepmaster commented Aug 26, 2017

shepmaster commented Oct 17, 2017

filipsch commented Oct 17, 2017

filipsch commented Oct 17, 2017

shepmaster commented Oct 17, 2017 •

edited

Loading

filipsch commented Oct 17, 2017

gokmen commented Oct 17, 2017

filipsch commented Oct 21, 2017

thlorenz commented Oct 27, 2017

filipsch commented Oct 30, 2017 •

edited

Loading

thlorenz commented Oct 30, 2017

thlorenz commented Oct 30, 2017

filipsch commented Oct 30, 2017

thlorenz commented Oct 30, 2017 •

edited

Loading

filipsch commented Oct 30, 2017

thlorenz commented Oct 30, 2017

shepmaster commented Oct 30, 2017

thlorenz commented Oct 30, 2017

LeviRosol commented Nov 9, 2017

filipsch commented Nov 9, 2017

LeviRosol commented Nov 10, 2017

update ace to v1.2.9; fix ts build; #93

update ace to v1.2.9; fix ts build; #93

Conversation

filipsch commented Jul 20, 2017

thlorenz commented Jul 20, 2017 • edited Loading

gokmen left a comment

Choose a reason for hiding this comment

gokmen Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

gokmen Jul 20, 2017

Choose a reason for hiding this comment

thlorenz Jul 21, 2017

Choose a reason for hiding this comment

gokmen Jul 21, 2017

Choose a reason for hiding this comment

filipsch Jul 22, 2017

Choose a reason for hiding this comment

thlorenz Jul 23, 2017

Choose a reason for hiding this comment

pankitgami commented Aug 22, 2017 • edited Loading

filipsch commented Aug 22, 2017 • edited Loading

pankitgami commented Aug 25, 2017

shepmaster commented Aug 26, 2017

shepmaster commented Oct 17, 2017

filipsch commented Oct 17, 2017

filipsch commented Oct 17, 2017

shepmaster commented Oct 17, 2017 • edited Loading

filipsch commented Oct 17, 2017

gokmen commented Oct 17, 2017

filipsch commented Oct 21, 2017

thlorenz commented Oct 27, 2017

filipsch commented Oct 30, 2017 • edited Loading

thlorenz commented Oct 30, 2017

thlorenz commented Oct 30, 2017

filipsch commented Oct 30, 2017

thlorenz commented Oct 30, 2017 • edited Loading

filipsch commented Oct 30, 2017

thlorenz commented Oct 30, 2017

shepmaster commented Oct 30, 2017

thlorenz commented Oct 30, 2017

LeviRosol commented Nov 9, 2017

filipsch commented Nov 9, 2017

LeviRosol commented Nov 10, 2017

thlorenz commented Jul 20, 2017 •

edited

Loading

gokmen Jul 20, 2017 •

edited

Loading

pankitgami commented Aug 22, 2017 •

edited

Loading

filipsch commented Aug 22, 2017 •

edited

Loading

shepmaster commented Oct 17, 2017 •

edited

Loading

filipsch commented Oct 30, 2017 •

edited

Loading

thlorenz commented Oct 30, 2017 •

edited

Loading