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

fixed relative paths for require.js #387

Closed
wants to merge 6 commits into from
Closed

fixed relative paths for require.js #387

wants to merge 6 commits into from

Conversation

Aaike
Copy link

@Aaike Aaike commented Jan 15, 2014

moved the require calls for require.js to be inside the define function so relative paths will work.

moved the require calls for require.js to be inside the define function so relative paths will work.
@bitwiseman
Copy link
Member

Do you have any suggestions for how we could implement tests for this functionality? We keep making modifications but they don't seem to land this area.

@Aaike
Copy link
Author

Aaike commented Jan 19, 2014

did you have unit tests in mind or a html test page that uses requirejs to load in the html beautifier and beautifies some code ?

@bitwiseman
Copy link
Member

I was hoping that since you have the expertise to fix the bug, you could show an example of it's use.

So, sure, if you can add an html test page that uses requirejs to load in the html beautifier that would be great.

@Aaike
Copy link
Author

Aaike commented Feb 11, 2014

Sorry did not have any time the last couple of weeks.

I looked into the issue again and it turns out that the error i was having only happens when using predefined path definitions with requirejs.

I made 2 examples, the first one loads in the html-beautifier directly in the require call using a relative path.
Doing it this way works without my modification.

requirejs test

The second example uses predefined path definitions for requirejs.
i.e :

require.config({
    paths:{
        "js-beautify": "../../lib/beautify",
        "css-beautify": "../../lib/beautify-css",
        "html-beautify": "../../lib/beautify-html"
    }
});

requirejs test with path definitions

If you were to run the second test without my modifications to beautify-html.js, requirejs would complain that it cannot find beautify.js and beautify-css.js.

Please verify this by putting the tests in your repository without updating beautify-html.js
I am actually not sure if this is a bug in requirejs or not :) But anyway moving the require calls inside the define function fixes this issue.

@Aaike
Copy link
Author

Aaike commented Feb 11, 2014

Also note that in the examples i have to do

var output = html_beautify.html_beautify(input);

Maybe it is better to return the html_beautify function directly instead of wrapping it in an object before returning it ?
So then it would become :

var output = html_beautify(input);

Just to be clear the define statement in beautify-html would have to become :

define(function() {
    js_beautify = require["./beautify.js"];
    css_beautify = require["./beautify-css.js"];
    return function(html_source, options) {
        return style_html(html_source, options, js_beautify, css_beautify);
      }
    ;
});

If you think that is a good idea i will go ahead and change it, and also change my examples.

@bitwiseman
Copy link
Member

Regarding your last comment about returning a function or an object: Take a look at #376. In that PR, the submitter seems to going the opposite direction adding an object wrapper around the function.

@Aaike
Copy link
Author

Aaike commented Feb 11, 2014

i see :) i don't have too much experience with node-js
so i guess the object wrapper should stay.

@bitwiseman bitwiseman modified the milestone: v1.5.0 Apr 25, 2014
@bitwiseman
Copy link
Member

I took the point you made here and made it work with the newly added requirejs tests. Kept your requirejs test page. Looks like it works. Thanks!

@bitwiseman bitwiseman closed this Apr 25, 2014
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.

2 participants