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

Move away from brace. #512

Closed
dennisoelkers opened this issue Aug 29, 2018 · 8 comments
Closed

Move away from brace. #512

dennisoelkers opened this issue Aug 29, 2018 · 8 comments

Comments

@dennisoelkers
Copy link
Contributor

Problem

react-ace is currently using brace as the underlying, browserified version of ace. Unfortunately brace is lagging a magnitude of versions behind and any change (feature, bug fix) to ace needs to propagate through brace before it ends up in react-ace. brace's development is currently stalling, with the last commit being done in February, issues not being answered and even PRs (like thlorenz/brace#139) not being accepted and merged. Therefore I would suggest looking for an alternative which allows to benefit from ace development.

References

Progress on: #

@securingsincity
Copy link
Owner

@dennisoelkers this would be a pretty big change. I'd be up for some help with this just as i don't know if i have the time to do this all myself

@dennisoelkers
Copy link
Contributor Author

@securingsincity I have a working branch ready and I am currently working on the remaining tests that fail. The change does not seem to be too big, so I am happy to help. What were your reasons for using brace instead of ace/ace-builds?

@securingsincity
Copy link
Owner

The interface was cleaner and early on it was easier to work with browserify

@securingsincity
Copy link
Owner

@dennisoelkers I ended up having to roll back that change. It looks like it breaks importing languages and themes. You can see on the examples that themes and languages no longer work after the switch to ace-builds

@dennisoelkers
Copy link
Contributor Author

dennisoelkers commented Sep 27, 2018

@securingsincity, thanks for the heads up! I actually verified that using the example when submitting the PR and just checked again. It works on my repository, at least as far as I can see. What is the error you are seeing?

@dennisoelkers
Copy link
Contributor Author

With "example", do you mean yarn run example or the example listed in the README for "basic usage"? The latter requires some changes for sure.

@manubb
Copy link

manubb commented Sep 27, 2018

The problem with the demos is probably a problem with github pages build.
(Locally, everything is allright.)
See: #538 (comment)
@securingsincity Could you try to merge again and see if the problem comes back?

@securingsincity
Copy link
Owner

This was addressed in v8

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 a pull request may close this issue.

3 participants