Skip to content

Conversation

@mik01aj
Copy link
Collaborator

@mik01aj mik01aj commented Nov 5, 2015

@sapegin, I need your help with testing this code. And I wait for any other comments. ;)

@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 5, 2015

I thought more about it, and it should be actually possible to infer the right context from the example code (rather than specifying the context regex upfront, for all examples at once). And for sure this would also be the most user-friendly way to do it. What do you think?

@sapegin
Copy link
Member

sapegin commented Nov 5, 2015

That would be very nice because now it’s hard to understand.

@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 9, 2015

Well, I researched this a bit, but apparently webpack doesn't expose its internal behaviour of creating a require contect containing whatever the module needs. The context API lets me create a context giving a regex, so I could look for /require\s*\((.*)\)/ in the example code and make a regex that would match any of the found strings and nothing else, but this seems clunky. It will work for the most important use case (require("string")), and I can provide some friendly error handling for the other cases so it won't bite the users much. Maybe it's still better than adding the hardly understandable config option.

What do you think?

@sapegin
Copy link
Member

sapegin commented Nov 9, 2015

I think it’s much better ;-)

@mik01aj mik01aj force-pushed the require-from-examples branch from a6d3e5d to a32b2cc Compare November 9, 2015 10:55
@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 9, 2015

Okay, see the new commit.

I'd add some tests for my helper functions but I don't see any test framework installed. Could you take care of this?

@sapegin
Copy link
Member

sapegin commented Nov 9, 2015

Cool! I’ll add Mocha soon.

@mik01aj mik01aj force-pushed the require-from-examples branch 2 times, most recently from 8c87b17 to facceef Compare November 9, 2015 15:05
@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 9, 2015

Check my new commit facceef! :)

@sapegin
Copy link
Member

sapegin commented Nov 9, 2015

Mocha is here!

@mik01aj mik01aj force-pushed the require-from-examples branch from facceef to e4af933 Compare November 10, 2015 14:32
@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 10, 2015

I thought about it again and I realized that I don't need the webpack's context at all. The context had also one drawback: requires of stuff from ../ didn't work. My new solution is simpler and cleaner, and requiring anything just works.

I still didn't add any tests, but I don't know what exactly could I test. Please look at the code and tell me if you feel like some part of it might break in the future.

@sapegin
Copy link
Member

sapegin commented Nov 10, 2015

Cool, thanks!

About tests:

  1. Complex regexps could be tested. It probably means extracting extracting them to utils.
  2. I thinks we can test that loader returns valid JavaScript (try to compile it with Babel?).

@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 12, 2015

extracting them to utils

This is harder than I thought because the loader is in ES5 and utils are ES6, so I can't just require('../src/utils/utils') from the loader.

@sapegin
Copy link
Member

sapegin commented Nov 12, 2015

Or you can just export these function from the loader and use them in tests.

@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 12, 2015

That's what I do:

module.exports = _.assign(exampleLoader, {
    requireAnythingRegex: requireAnythingRegex,
    simpleStringRegex: simpleStringRegex,
    readExamples: readExamples,
    findRequires: findRequires,
});

Btw, there's a simpler way to check JS (ES5) validity: http://www.ecma-international.org/ecma-262/5.1/#sec-15.3.2.1 :

If body is not parsable as FunctionBody then throw a SyntaxError exception.

@sapegin
Copy link
Member

sapegin commented Nov 12, 2015

Cool! And a simpler way is always better ;-)

@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 12, 2015

Done!

@mik01aj mik01aj force-pushed the require-from-examples branch from cdc6736 to 999a30c Compare November 12, 2015 11:37
@sapegin
Copy link
Member

sapegin commented Nov 12, 2015

Cooooool!

@mik01aj mik01aj force-pushed the require-from-examples branch from 999a30c to 828436c Compare November 12, 2015 11:41
@mik01aj
Copy link
Collaborator Author

mik01aj commented Nov 12, 2015

commit updated, ihmo it's good to merge now.

sapegin added a commit that referenced this pull request Nov 12, 2015
Added support for using require(...) in code examples.
@sapegin sapegin merged commit 706249d into styleguidist:master Nov 12, 2015
@sapegin
Copy link
Member

sapegin commented Nov 12, 2015

Thank you very much!

:shipit:

@mik01aj mik01aj deleted the require-from-examples branch November 12, 2015 12:51
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