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

Replace calls to cheerio with a native JS implementation #321

Closed
julianlam opened this issue Sep 22, 2013 · 6 comments
Closed

Replace calls to cheerio with a native JS implementation #321

julianlam opened this issue Sep 22, 2013 · 6 comments
Assignees
Milestone

Comments

@julianlam
Copy link
Member

Every time we render a post (postTools.toHTML), we utilise cheerio to break down the fully rendered HTML into element objects, find the anchors, replace its href to send the user to that NodeBB's /outgoing route, and then re-render the code again back into HTML.

Cheerio, being a synchronous utility, is not the ideal solution for this.

Questions/Tasks

  1. Do we even need an /outgoing page, has it fallen out of favour?
  2. Should the /outgoing page be made a plugin instead?
    • If so, the plugin system needs the ability to add new routes dynamically
  3. If yes, replace the single call to cheerio with a native JS regexp implementation
  4. Remove cheerio as a dependency
@ghost ghost assigned julianlam Sep 22, 2013
@julianlam
Copy link
Member Author

Paging @psychobunny re: Question 1.

@psychobunny
Copy link
Contributor

I realize there was a comment about someone not liking it earlier, but its actually a pretty useful thing to have. Maybe not necessarily in the core though (or having the ability to switch on/off)

Maybe the optimal thing to do is to replace the link upon saving, what do you think?

@julianlam
Copy link
Member Author

That would be a good solution for me, but I am a huge fan of not mucking around with a user's post.

In short, if I post something, then edit it, the raw text that is returned to me should be the exact same text that I input earlier, and altering the urls on-save would break this "rule" (note the quotes).

How about this: We don't even bother changing the URL altogether (so if the user hovers over the url, it'll be the actual link), but we have some js on the client side:

$(document.body).on('click', 'a', function() {
    if (internal link) ajaxify.go();
    else document.locaton('/outgoing...');
});

In fact, I believe the basic code is already in ajaxify to detect if a href is #, in order to do nothing, right? You could extend it so that urls not going to the same domain will send the user to the /outgoing route instead.

@julianlam
Copy link
Member Author

Take a look at James Padolsey's Parsing URLs with the DOM!.

this.hostname is already present, so there's probably even no need to do regex trickery to find the domain...

@psychobunny
Copy link
Contributor

this is the route that seems the best, hope that's what you ended up doing:

In fact, I believe the basic code is already in ajaxify to detect if a href is #, in order to do nothing, right? You could extend it so that urls not going to the same domain will send the user to the /outgoing route instead.

@julianlam
Copy link
Member Author

Yeah, I just compare the domain (might have to check port too, now that I think about it)

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

No branches or pull requests

2 participants