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

Concatenating js into one file, not finding the jsoneditor theme & mode files #157

Closed
andrewboni opened this issue Jan 20, 2015 · 14 comments

Comments

@andrewboni
Copy link

Hi, as part of my build process, I'm concatenating all of my js files into a single file, which ends up living outside of my /public/bower_components/ dir, where jsoneditor and other js libraries live.

As a result, the theme and formatting files are 404ing:

Failed to load resource: https://app.mydomain.com/mode-json.js the server responded with a status of 404 (Not Found)
Failed to load resource: https://app.mydomain.com/theme-jsoneditor.js the server responded with a status of 404 (Not Found)

Is there a way to explicitly specify the path to mode-json.js and theme-jsoneditor.js ? Or can jsoneditor.min.js include these files automatically? What's the best way to handle this? Thanks!

@josdejong
Copy link
Owner

Yes, I would love to bundle these files inside jsoneditor.js. So far it didn't work because Ace editor tries to load these files dynamically, which can be problematic. See also #87.

There may be ways to neatly bundle the files that ace editor needs in one file. but I haven't yet succeeded in that. If you find a way to solve this please let me know!

@nfvs
Copy link
Contributor

nfvs commented Feb 23, 2015

I've actually been able to get rid of every warning except the theme one by using Brace (https://github.com/thlorenz/brace), since I'm using browserify.

Is there a way to change the theme in jsoneditor? Brace comes with multiple ones but jsoneditor always tries to load theme-jsoneditor.js.

@josdejong
Copy link
Owner

Ah, that's good to hear. Which warning is remaining exactly, and is that because of the current build of JSONEditor, or due to Ace itself?

Time to have another look at brace (see also #64). Right now the code of JSONEditor is AMD (published as UMD), I choose that at that time partly to get experience with AMD, partly because Ace was using AMD, and partly because I didn't have a strong opinion on which module system to use. But right now JSONEditor is the only project I have left using AMD. In other projects I'm using CommonJS + Browserify/WebPack everywhere, with great satisfaction.

Refactoring to CommonJS will not be that much work.

@nfvs
Copy link
Contributor

nfvs commented Feb 24, 2015

That's my experience as well :-)

Anyway this is the error, trying to find the theme-jsoneditor.js module/file:

GET http://localhost:5000/theme-jsoneditor.js 

Caused by this line I suppose:

editor.setTheme('ace/theme/jsoneditor');

I believe that if there was a way to set the theme, perhaps in the constructor options, it would solve this.

I have also tried to require jsoneditor/asset/ace/theme-jsoneditor.js but get multiple errors with Browserify, so I ended up ignoring that error for now.

@josdejong
Copy link
Owner

Ok thanks for the feedback. Let's give that a try.

When I load example 03_switch_mode and switch to mode "code", I don't see a network request to theme-jsoneditor.js. This may be because I've packed this file inside /asset/ace/ace.js, which is loaded in the browser before editor.setTheme('ace/theme/jsoneditor'); is called. Did you load the file theme-jsoneditor.js before instantiating the editor (maybe using a simple script tag just for testing)?

@nfvs
Copy link
Contributor

nfvs commented Feb 24, 2015

I did not, because I am packing all vendor dependencies into a single minified vendor.min.js and wanted to avoid loner scripts.

Trying to include jsoneditor/asset/ace/theme-jsoneditor.js in there with browserify results in ../lib/dom not being found, I'm guessing because format of theme-jsoneditor.js is not compatible with browserify (which is the reason that Brace was created in the first place).

I also don't think jsoneditor is loading Ace in this case, I'm pretty sure Brace is overriding all usages of Ace, so the only leftover trace of Ace is the theme name which doesn't exist in Brace.

If you look at Brace's usage example, you'll see that in my case Brace is dynamically looking for a theme named json-editor, which doesn't exist in Brace. This could be solved by 1) including json-editor in Brace, or 2) providing an override for setTheme('ace/theme/json-editor'), so we could use for example 'ace/theme/monokai', which does exist in Brace.

@josdejong
Copy link
Owner

Yes, but I meant just for testing if this works out. The theme-jsoneditor.js file is indeed AMD, so it doesn't work out of the box. I hope to experiment with this myself soon.

@josdejong
Copy link
Owner

I've reworked the source code from AMD to CommonJS modules and now use brace. The refactoring went extremely smooth, the complete refactoring took only 30 mins. Unbelievable.

@andrewboni and @nfvs can you maybe give the jsoneditor in the develop branch a try?

@nfvs
Copy link
Contributor

nfvs commented Mar 11, 2015

@josdejong 4.0.0 is not working for me. The index.js file is using require('./src/js/JSONEditor'), but the distribution version installed with npm doesn't contain the src/ folder, only dist/.

Fatal error: Cannot find module './src/js/JSONEditor' from '/node_modules/jsoneditor'

@nfvs
Copy link
Contributor

nfvs commented Mar 11, 2015

The solution to this would be to include src in the npm package, since a common usage is to use browserify to build a distribution vendor.js with all dependencies (including brace and jsoneditor).

@josdejong
Copy link
Owner

Ah good point! Will fix it, thanks.

For now I guess you could load the editor via require('jsoneditor/dist/jsoneditor') instead of require('jsoneditor').

@nfvs
Copy link
Contributor

nfvs commented Mar 12, 2015

I think dist/jsoneditor is merely the "everything-and-the-kitchen-sink" that you just need to include as a script in your website and it works out of the box.

It's not a browserified package though, so I believe there should be another build output dist/jsoneditor-browserify.js that index.js references, to be used as a package. I've started creating a gulp task using browserify for that, but hit the problems referenced in issue #166, where the output of browserify src/js/JSONEditor.js is not usable.

@josdejong
Copy link
Owner

./dist/jsoneditor.js is generated as an UMD package (Same with the minified version). It can be used in the browser, via AMD, and via CommonJS. So you can use it perfectly fine via browerify, no need for a specialdist/jsoneditor-browserify.js. (which isn't needed anyway of course as soon the src directory is added and you can load jsoneditor the regular way via ./index.js).

@josdejong
Copy link
Owner

@nfvs I've just released jsoneditor v4.1.2, which includes the src folder on both npm and bower, so that should make it possible to use browserify directly without having to clone the project.

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

3 participants