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

Allow using a custom build of Ace #82

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

tqc
Copy link

@tqc tqc commented Apr 11, 2017

Adds the option to build from Ace source rather than the ace-builds repo, which makes it much easier to debug ace in an app that requires brace.

To use the local/custom version of ace, run

npm link /path/to/ace

Note that the output will not be quite the same as obtained from ace-builds, but that appears to be an issue with ace-builds not removing files that no longer exist in the source.

@thlorenz
Copy link
Owner

thlorenz commented Apr 11, 2017

I like the idea, but would prefer this being explicit, i.e. --acedir ./ace flag.
This way it's obvious instead of magically happening if you happen to have a specific directory in the current folder.

Feel free to pull in a command line tool like minimist to parse the args.
Also make sure to pass along any args passed to the npm update script.
That way I could do npm run update --acedir ./ace

Thanks.

@tqc
Copy link
Author

tqc commented Apr 12, 2017

Adding a full command line for this seems like overkill to me.

Certainly it needs to be documented, but it isn't just putting ace in an arbitrary magic path.

I was going for the conventional behavior of an npm dev/optional dependency - not included with a default npm install, but can be placed in the standard location by running npm install/link and will be used if available.

This approach also opens up possibilities like including ace in devDependencies, though unfortunately ace source isn't published to npm, and npm update is a little flaky with git dependencies.

@thlorenz
Copy link
Owner

I'm afraid that someone may inadvertently have that folder in their path, possibly from a previous update script run that didn't finish or whatever and then unexpectedly the script uses that without being explicit about it (console.log isn't sufficient).
I don't like magic and prefer users to opt in into using a local version.

We don't need minimist necessarily, but at least a flag (you can parse the args manually).

@tqc
Copy link
Author

tqc commented Apr 12, 2017

Pretty sure that can't happen. It's in node_modules, so the only way it can legitimately get there is explicitly adding it with npm install or nom link - essentially a config command saying to use this source for all future builds.

Unlike the ace-build folder, it is not created by the script, always manually.

@thlorenz
Copy link
Owner

Ah ok I think I'm not understanding correctly, you want to build this manually after you installed brace?
I though this is so you can create your custom version, publish it and install that.

If the latter is true that's a very special use case isn't it? It's the first time I hear that anyone wants to do that.

makes it much easier to debug ace in an app that requires brace

How so? You're still gonna run the bundled together code, how could you step into the original code?

@tqc
Copy link
Author

tqc commented Apr 12, 2017

That was npm install in the dev environment sense - clone brace from git, npm install to get dependencies, then npm install github:tqc/ace or link to add the optional dependency which isn't in package.json.

I wouldn't expect update to be run when using npm install brace from another package, but it does work nicely when using npm link brace so that my dev server gets the customised version of brace.

Using a custom build of ace isn't something many people would need to do, but I happen to need some features / fixes that aren't in the currently released version - specialized, but still a legitimate use case.

I meant debugging in the sense of being able to run it at all without going through the full publish process - sourcemaps are nice, but not essential.

@sstepper
Copy link

This is exactly what I needed. It enabled me to use custom language files. I guess the implementation details just need working out to merge the PR, right?

@CAspeling
Copy link

This would allow us to use minified ace? How else can we make brace smaller?

fxmontigny/ng2-ace-editor#108 (comment)

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 this pull request may close these issues.

4 participants