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

`require(*tag-name*)` does not allow `parserOptions` #1935

Closed
bonpixel opened this Issue Aug 10, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@bonpixel

bonpixel commented Aug 10, 2016

  1. Describe your issue:
    Requiring a tag via require does not allow parserOptions to be set. This affects tags that require custom options to compile.

    More Background Below

  2. Can you reproduce the issue?
    Not via plunkr, this is a server-side issue.

  3. On which browser/OS does the issue appear?
    Node

  4. Which version of Riot does it affect?
    All

  5. How would you tag this issue?

    • Question
    • Bug
    • Discussion
    • Feature request
    • Tip
    • Enhancement
    • Performance

Background

I've been trying to utilize riot's ability for server-side rendering http://riotjs.com/guide/#server-side-rendering

let riot = require( 'riot' );
let myTag = require( './myTag' );

BUT myTag in this example is also using a custom style type of scss and a node-sass plugin bourbon.

<myTag>
    <div>
        <p>Hello World!</p>
    </div>
    <style type="scss" scoped>
        @import "bourbon";
        p { color: blue; }
    </style>
</myTag>

When I try to bring in the file via require, it fails since it doesn't know what bourbon is

Error: File to import not found or unreadable: bourbon

I can see that require here ultimately calls riot.compile on the tag

@ riot/lib/server/index.js:12:1

// allow to require('some.tag')
require.extensions['.tag'] = function(module, filename) {
  var src = riot.compile(require('fs').readFileSync(filename, 'utf8'))
  module._compile(
    'var riot = require(process.env.RIOT || "riot/riot.js");module.exports =' + src
  , filename)
}

but there is not a way to pass in opts with specific parserOptions when using require.

Is there another way that I can load a tag into a file server-side that would allow me to pass in opts or, better yet, is there a way that I can set the parserOptions at a global level? (specifically I need to set the includePaths to include the bourbon plugin path)

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Aug 11, 2016

Member

@bonpixel do you have any idea or suggestions about the syntax we need? I have no idea to pass the option through require()...

An easy and solid workaround is precompiling the tags into JavaScript files.
Or custom parsers could help you, I think.
http://riotjs.com/api/compiler/#a-namejs-parsera-riotparsersjs-js-options

Member

cognitom commented Aug 11, 2016

@bonpixel do you have any idea or suggestions about the syntax we need? I have no idea to pass the option through require()...

An easy and solid workaround is precompiling the tags into JavaScript files.
Or custom parsers could help you, I think.
http://riotjs.com/api/compiler/#a-namejs-parsera-riotparsersjs-js-options

@bonpixel

This comment has been minimized.

Show comment
Hide comment
@bonpixel

bonpixel Aug 11, 2016

@cognitom It seems to me that require isn't going to provide a mechanism to pass options into.
You can see that it only accepts one parameter in the node source code

My thought here was this: Riot already provides a way to add opts directly to a call to compile. What if Riot also provided a global / config way to add options to parsers programmatically? Something like what can be used with the CLI: http://riotjs.com/guide/compiler/#es6-config-file? It would be cool to provide Riot something after it is required to configure it. Something like:

let riot = require( 'riot' )({
    parserOptions: {
        style: {
            includePaths: require( 'node-bourbon' ).includePaths
        }
    }
});

Do you agree?

I do understand your point about pre-compiling the tag but I was hoping to avoid that. The main reason is that I will need to create a lot of glue code to make that possible. That is an option of course but will make what was otherwise a simple concept more complex.

To provide a little more background information on what lead me to this "issue/enhancement": I am trying to do unit testing for components. Here is the example I was trying to go for:

mytag.spec.js

"use strict";

let riot    = require( 'riot' );
let cheerio = require( 'cheerio' );
let expect  = require( 'expect.js' );
let comp    = require( './myTag' );  // Here is where the error happens when the tag has a custom plugin like `bourbon`

describe( '<myTag> ::', () => {

    let tag, $;

    before( () => {
        tag = riot.render( comp );
        $ = cheerio.load( tag );
    });

    describe('Anchor', () => {

        it( 'Contains an anchor', () => {
            expect( $( 'a' ).length ).to.be( 1 );
        });

        it( 'Contains an alt tag', () => {
            expect( $( 'a' ).attr( 'alt' ) ).to.be.ok();
        });

        it( 'Uses either a default or dynamic value for alt text', () => {
            expect( $( 'a' ).attr( 'alt' ) ).to.be( 'My Alt Text' );

            tag = riot.render( comp, { alt: 'Dynamic Value' });
            $ = cheerio.load( tag );
            expect( $( 'a' ).attr( 'alt' ) ).to.be( 'Dynamic Value' );
        });
    });
});

I wrote this because I want to be able to unit test components in isolation. The above code is really straightforward and readable which is awesome. I just run into trouble where <myTag> contains any special plugins for the compilers.

bonpixel commented Aug 11, 2016

@cognitom It seems to me that require isn't going to provide a mechanism to pass options into.
You can see that it only accepts one parameter in the node source code

My thought here was this: Riot already provides a way to add opts directly to a call to compile. What if Riot also provided a global / config way to add options to parsers programmatically? Something like what can be used with the CLI: http://riotjs.com/guide/compiler/#es6-config-file? It would be cool to provide Riot something after it is required to configure it. Something like:

let riot = require( 'riot' )({
    parserOptions: {
        style: {
            includePaths: require( 'node-bourbon' ).includePaths
        }
    }
});

Do you agree?

I do understand your point about pre-compiling the tag but I was hoping to avoid that. The main reason is that I will need to create a lot of glue code to make that possible. That is an option of course but will make what was otherwise a simple concept more complex.

To provide a little more background information on what lead me to this "issue/enhancement": I am trying to do unit testing for components. Here is the example I was trying to go for:

mytag.spec.js

"use strict";

let riot    = require( 'riot' );
let cheerio = require( 'cheerio' );
let expect  = require( 'expect.js' );
let comp    = require( './myTag' );  // Here is where the error happens when the tag has a custom plugin like `bourbon`

describe( '<myTag> ::', () => {

    let tag, $;

    before( () => {
        tag = riot.render( comp );
        $ = cheerio.load( tag );
    });

    describe('Anchor', () => {

        it( 'Contains an anchor', () => {
            expect( $( 'a' ).length ).to.be( 1 );
        });

        it( 'Contains an alt tag', () => {
            expect( $( 'a' ).attr( 'alt' ) ).to.be.ok();
        });

        it( 'Uses either a default or dynamic value for alt text', () => {
            expect( $( 'a' ).attr( 'alt' ) ).to.be( 'My Alt Text' );

            tag = riot.render( comp, { alt: 'Dynamic Value' });
            $ = cheerio.load( tag );
            expect( $( 'a' ).attr( 'alt' ) ).to.be( 'Dynamic Value' );
        });
    });
});

I wrote this because I want to be able to unit test components in isolation. The above code is really straightforward and readable which is awesome. I just run into trouble where <myTag> contains any special plugins for the compilers.

@cognitom cognitom added the discussion label Aug 12, 2016

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Aug 12, 2016

Member

As I have already mentioned here I would like to add riot.require|riot.register|riot.load to allow requiring tags also with options. I think I could work on it soon

Member

GianlucaGuarini commented Aug 12, 2016

As I have already mentioned here I would like to add riot.require|riot.register|riot.load to allow requiring tags also with options. I think I could work on it soon

@bonpixel

This comment has been minimized.

Show comment
Hide comment
@bonpixel

bonpixel Aug 12, 2016

@GianlucaGuarini an official loader sounds like an excellent solution.

I noticed too that it was deprecated in the Node docs but the documentation seemed to indicate that it wasn't really going to change. I like the idea of an official loader better though.

for now, I came up with a hacky work around of extending the compile method:

let riot    = require( 'riot' );
let _    = require( 'lodash' );

let tmp = riot.compile;
riot.compile = function(){
    // (src, opts, url)
    var args = arguments;
    return tmp.call(riot, args[0], _.extend({
        parserOptions: {
            style: {
                includePaths: require( 'node-bourbon' ).includePaths
            }
        }
    }, args[1]), args[2]);
};

Not my favorite thing to do, but it works.

bonpixel commented Aug 12, 2016

@GianlucaGuarini an official loader sounds like an excellent solution.

I noticed too that it was deprecated in the Node docs but the documentation seemed to indicate that it wasn't really going to change. I like the idea of an official loader better though.

for now, I came up with a hacky work around of extending the compile method:

let riot    = require( 'riot' );
let _    = require( 'lodash' );

let tmp = riot.compile;
riot.compile = function(){
    // (src, opts, url)
    var args = arguments;
    return tmp.call(riot, args[0], _.extend({
        parserOptions: {
            style: {
                includePaths: require( 'node-bourbon' ).includePaths
            }
        }
    }, args[1]), args[2]);
};

Not my favorite thing to do, but it works.

@adrianblynch

This comment has been minimized.

Show comment
Hide comment
@adrianblynch

adrianblynch Sep 8, 2016

@bonpixel - We do something similar.

We read the tag file from disk and together with our parser options, pass it into riot.compile().

adrianblynch commented Sep 8, 2016

@bonpixel - We do something similar.

We read the tag file from disk and together with our parser options, pass it into riot.compile().

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Sep 8, 2016

Member

Guys you could try my patch I guess it should not be hard to include it in your workflow. 8f24714

I guess riot.require will land in riot@3.0.0

Member

GianlucaGuarini commented Sep 8, 2016

Guys you could try my patch I guess it should not be hard to include it in your workflow. 8f24714

I guess riot.require will land in riot@3.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment