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.

Sign up for GitHub

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

Jump to bottom

Replace JSdom with Cheerio #99

Closed
robhrt7 opened this issue Feb 11, 2015 · 12 comments
Closed

Replace JSdom with Cheerio #99

robhrt7 opened this issue Feb 11, 2015 · 12 comments
Assignees
@robhrt7
Labels
improvement
Milestone
0.6.0

Comments

@robhrt7
Copy link
Member

robhrt7 commented Feb 11, 2015

To achieve more pleasant experience of SourceJS installation and to do stuff more efficiently in terms of performance, we chose migration to https://github.com/cheeriojs/cheerio from JSdom.

Current modules that needs to be refactored:

  • Clarify - uses JSdom to run universal spec parsing script. Will be most difficult to rewrite
  • read.js middleware - adds some runtime processing of Specs before sending the response, could be easily rewritten to use cheerio
  • index.md support - will use cheerio from first release
@robhrt7
Copy link
Member Author

robhrt7 commented Feb 17, 2015

Index.md support already uses cheerio #100

@robhrt7 robhrt7 added this to the 0.6.0 milestone Apr 26, 2015
@robhrt7 robhrt7 mentioned this issue Apr 26, 2015
Closed
@tcorral
Copy link

tcorral commented May 13, 2015

Could you @operatino assign me this issue, please? I could not do it by myself.

@robhrt7
Copy link
Member Author

robhrt7 commented May 13, 2015

Done! I've also added you to contributors groups, so you now should be able to interact with tasks. If you chose this task to solve, come over to discuss the details.

Or you can first jump in and see how it works by yourself, here are the files to check:

  • sourcejs/core/middleware/clarify.js - gets the spec page, runs client side parser (window.SourceGetSections) and returns an object with HTML parts. SourceGetSections is located in source/assets/js/modules/sectionsParser.js, and should be runnable on server side, so it should be also refactored. sectionsParser is used by Clarify, client side API sync method, and in PhantomJS env, for caching all the rendered HTML results into API.
  • sourcejs/core/middleware/wrap.js - this is easier, it's just searching for <head> tag, and if there is and element for that, it passes it's contents to EJS templating engine

Just for SourceJS and when you're ready, create PR to get feedback, or just catch me in the office.

@robhrt7
Copy link
Member Author

robhrt7 commented May 16, 2015

Branch with temp JSDom cleanup - 0.5.3-bb. Clarify feature is turned off there, as a temp solution. And we also need to re-check how pages with duplicated head are rendered.

@robhrt7
Copy link
Member Author

robhrt7 commented May 19, 2015

I replaced JSDom with cheerio in operatino/no-jsdom-in-wrap branch. Spec is now loading much faster as well, keeping <head> section feature, and HTML normalization, same as we had in JSDom.

But I'm planning to remove all those custom processing from wrap.js in 0.6.0, moving them to plugins. Most probably I will create two small plugins based on cheerio and jsdom.

@tcorral
Copy link

tcorral commented May 26, 2015

Hi @operatino . Could you check why the tests are not working in master branch? I am trying to execute the test to know if the changes are ok and work as expected but seems that something is broken in master.

@robhrt7
Copy link
Member Author

robhrt7 commented May 27, 2015

@tcorral, you need to run the app first, and then trigger the tests, they should be passing. In 0.5.3 I already updated npm test, so it automatically runs the app before the tests.

@tcorral
Copy link

tcorral commented May 31, 2015

I have tried to make a clean checkout of 0.5.3-dev branch and execute the npm test but I get this error 'ENOENT, no such file or directory '/Users/tomascorralcasas/WebStormProjects/Source/user'

@robhrt7
Copy link
Member Author

robhrt7 commented May 31, 2015

Just follow the install instructions to get right SourceJS set-up http://sourcejs.com/docs/base/#install, or in your case, you will need to just clone this repo in sourcejs/user folder https://github.com/sourcejs/init.

BTW, 0.5.3 is already released and 0.5.3-dev branch is now deleted, I have left only freezed tag for it.

And good point here, I will try to make Sourcejs testable without user folder as well.

@robhrt7
Copy link
Member Author

robhrt7 commented May 31, 2015

Reach me through Skype haritonov.r or email r@rhr.me for more details.

@tcorral
Copy link

tcorral commented May 31, 2015

ok

2015-05-31 2:07 GMT-07:00 Robert Haritonov notifications@github.com:

Reach me through Skype haritonov.r or email r@rhr.me for more details.

—
Reply to this email directly or view it on GitHub
#99 (comment).

robhrt7 added a commit that referenced this issue Sep 28, 2015
@robhrt7
#99: initial spec parser version on CHeerion
ddcba17
robhrt7 added a commit that referenced this issue Sep 28, 2015
@robhrt7
#99: finish section parsing script
21b77d7
@robhrt7 robhrt7 assigned robhrt7 and unassigned tcorral Sep 28, 2015
robhrt7 added a commit that referenced this issue Sep 28, 2015
@robhrt7
#99: working clarify with cheerio, and selection by section
ff8429b
robhrt7 added a commit that referenced this issue Oct 3, 2015
@robhrt7
#99: add resource parsing and tests, move partials to tests root folder
d066055
robhrt7 added a commit that referenced this issue Oct 3, 2015
@robhrt7
#99: removing last bits of jsdom
cee60d1
@robhrt7
Copy link
Member Author

robhrt7 commented Oct 3, 2015

Done

@robhrt7 robhrt7 closed this as completed Oct 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@robhrt7 robhrt7

Labels
improvement
Projects
None yet
Milestone
0.6.0
Development

No branches or pull requests
2 participants
@tcorral @robhrt7