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

Use browserify to bundle index.js instead of webpack, don't include brace by default. #166

Closed
nfvs opened this issue Mar 11, 2015 · 20 comments

Comments

@nfvs
Copy link
Contributor

nfvs commented Mar 11, 2015

The default build in dest/jsoneditor.js includes brace and jsonlint, which seems to create many problems when using browserify to bundle jsoneditor due to some missing libs (e.g. there's no file module with browserify, which makes jsonlint choke).

Apparently webpack doesn't easily support excluding libraries, so perhaps using browserify to create a clean jsoneditor.js without bundled dependencies would be best. This resulting jsoneditor.js contents can either go into index.js (which browserify tries to include by default), or into a new file in dist/, which package.json can reference with the main option.

nfvs added a commit to nfvs/jsoneditor that referenced this issue Mar 11, 2015
@josdejong
Copy link
Owner

I think the only problem is with jsonlint not working with browserify right? Or are there problems with brace too? Webpack certainly supports excluding libraries, that's not the problem.

The offered jsonlint package is indeed sort of broken, the "file" module it requires is missing. Webpack doesn't fail on missing modules, but browserify does. To bundle with browserify: https://github.com/josdejong/jsoneditor#custom-builds.

I've created an issue at jsonlint: zaach/jsonlint#57

If the issue isn't resolved by jsonlint we can always just copy jsonlint, fix it, put it in an /asset folder and use that one instead of the one from npm.

@nfvs
Copy link
Contributor Author

nfvs commented Mar 12, 2015

Actually I'm pretty sure that's not the only problem.

Browserify is not building the lib, even when you use --no-bundle-external, or -x jsonlint. You can tell because the instructions always include --ignore-missing, which is a nuclear option and merely comments out all the require() calls that are not working, ensuring that something always get built, not necessarily what you were expecting.

I have tried all kinds of combinations, using gulp, using browserify src/js/JSONEditor.js -o jsoneditor-browserify.js --no-bundle-external, nothing works.

@nfvs
Copy link
Contributor Author

nfvs commented Mar 12, 2015

Ironically enough, version 3.2.0 works perfectly with Browserify.

@josdejong
Copy link
Owner

Ironically enough, version 3.2.0 works perfectly with Browserify.

Yes, there was quite a big change in the code structure, hence the 4.0.0 versioning :). The code was changed from AMD to CommonJS modules, and now uses brace and jsonlint directly from npm. No more custom concatenation of special, hand copied assets.

I totally agree with you that the need to use --no-bundle-external is a bad thing, we have to solve that (i.e. jsonlint should get fixed).

Did you use the source code cloned from github to do the custom build? Or did you use the package installed via npm (which is currently missing the src folder, see your issue #157)?

@nfvs
Copy link
Contributor Author

nfvs commented Mar 12, 2015

I think --no-bundle-external is a good thing (when building a distribution version of jsoneditor), since you're not redistributing other libraries and are just relying on them to be there when they get used. On the other hand, --ignore-missing is definitely bad since it seems to be ignoring "compilation errors".

I built it from github, the npm package is broken.

If you build with browserify dist/jsoneditor.js ..., all browserify parameters will be ignored since that already contains the whole library including brace/jsonlint. However that's not a "browserify-compatible" module.

If you build with browserify src/js/JSONEditor.js ..., when you use the lib you'll encounter many Cannot find module ./util and Cannot find module ./treemode errors.

To make sure I'm not doing something wrong, try using the output of the examples in the README, to create a JSONEditor object.

@josdejong
Copy link
Owner

Sorry I mean't --ignore-missing instead of --no-bundle-external.

hm that's odd. I just did the following to test this, working just fine:

  • go to a tmp directory
  • run git clone https://github.com/josdejong/jsoneditor.git
  • cd jsoneditor
  • npm install
  • Run browserify in the root of the project:
    browserify ./index.js -o ./jsoneditor.custom.js -s JSONEditor --ignore-missing
    Tested with ./src/js/JSONEditor.js as enty file too, works fine too.
  • after that, browserify has generated a file jsoneditor.custom.js
  • open ./examples/01_basic_usage.html in a text editor, replace the script url ../dist/jsoneditor.js with ../jsoneditor.custom.js
  • open ./examples/01_basic_usage.html in the browser, see if the editor works ok.

@nfvs
Copy link
Contributor Author

nfvs commented Mar 12, 2015

Does the "code" mode work too?

@josdejong
Copy link
Owner

Tested that too, works indeed fine. The generated jsoneditor.custom.js is ~887 KB

@josdejong
Copy link
Owner

@nfvs Did you already figure out what was going wrong with browserify?

@nfvs
Copy link
Contributor Author

nfvs commented Mar 15, 2015

Not really, building a single vendor.js with browserify containing multiple external libs fails on jsoneditor, so I'm using 3.2.0 which works. No idea how to fix it so I guess we can close this.

@nfvs nfvs closed this as completed Mar 15, 2015
@josdejong
Copy link
Owner

I still have to fix the jsonlint related stuff, after that things should run smooth and should not require this --ignore-missing flag. But these other errors you get with browserify are weird.

@josdejong
Copy link
Owner

I've released v4.1.3, which fixes the broken dependencies of jsonlint. You should now be able to use browserify the regular way, without need for special command line options

https://github.com/josdejong/jsoneditor#custom-builds

@nfvs
Copy link
Contributor Author

nfvs commented Mar 17, 2015

Ah, the common "screw it, I'll use my own fork!" workaround :-)

Thanks for the work, I will try this first thing tomorrow.

@josdejong
Copy link
Owner

yes, don't we all love github? ;)

I will do a PR to the original jsonlint project, but it doesn't look very alive lately so we have to see.

@nfvs
Copy link
Contributor Author

nfvs commented Mar 18, 2015

Still getting a WARNING: Cannot load code editor, Ace library not loaded. Falling back to plain text editor as before, even though I'm also require()ing brace myself.

@josdejong
Copy link
Owner

Thanks for testing. This warning occurs when brace is excluded from the jsoneditor bundle. In v3 you had to load Ace separately, but in v4 it's bundled inside jsoneditor (it will not recognize any globally loaded Ace editor).

Can you get a custom build of jsoneditor working outside of your project? Like creating a clone of the github project in a tmp directory on your computer, and then running browserify in the root of the project:

browserify ./index.js -o ./jsoneditor.custom.js -s JSONEditor

Then adjust one of the examples to use jsoneditor.custom.js instead of the provided one and see if it works. Does that work for you?

@nfvs
Copy link
Contributor Author

nfvs commented Mar 18, 2015

When doing that separately it works, and the examples also work, but in my own project (it's using grunt-browserify to build a vendor.js) it's failing for some reason. I'm trying to find out why.

@nfvs
Copy link
Contributor Author

nfvs commented Mar 18, 2015

Ok I think I found the problem.

When I include both brace and jsoneditor in the same vendor.js, I get a TypeError in the try/catch block in the beginning of textmode.js:

Cannot read property 'ace/ace' of undefined.

Seems it's a brace error, not jsoneditor, but this is weird especially because if I remove require('brace') from vendor.js, no such error occurs.

This is apparently not a problem if we're not including brace ourselves outside of jsoneditor, but if for some reason we are (e.g. for using a different theme in ace), then this error pops up.

Still not 100% sure this is due to some error on my part, I'm still trying to find that out, but if not it may be useful to allow overriding of the (br)ace object in jsoneditor, the same way it happens with Backbone.js:

$ = require('jquery');
Backbone = require('backbone');
Backbone.$ = $;

@josdejong
Copy link
Owner

may be useful to allow overriding of the (br)ace object in jsoneditor

That's a good idea, thanks.

@nfvs
Copy link
Contributor Author

nfvs commented Mar 18, 2015

I managed to fix that problem with some browserify magic btw, but allowing overrides may still be a good idea. Perhaps by first checking if a this.ace object exists, if so assume it's a brace module and use it, if not, this.ace = require('./ace').

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

No branches or pull requests

2 participants