paypal / react-engine

a composite render engine for universal (isomorphic) express apps to render both plain react views and react-router views
Apache License 2.0
1.45k stars 130 forks source link

If render url contains period "Error: Cannot find module" #52

Open reggi opened 9 years ago

reggi commented 9 years ago

When your using express the docs use res.render(req.url, data) the following to render the url to react. This is causing an issue anytime there is a period in the url.

I have periods in my get parameters and in my route. You can pass req.path to pass only the path to the router. There is also periods in the url like /index.html the error is Error: Cannot find module 'html'.

Do I have to escape periods to the url? res.render(req.url.replace(/\./g, ""), data)

reggi commented 9 years ago

The expressView function uses express/lib/view.

It seems like express view accepts a string path for its first parameter. It tries to parse out the extension of that path and load its templating engine (ex. index.ejs, index.jade). When I pass it something like /index.html it thinks it's looking for the html template. When I pass it something like /index?url=google.com it thinks that com is the template engine.

Is there a reason react-engine doesn't support periods? If not shouldn't the examples be something like:

res.render(req.path.replace(/\./g, ""), data)
reggi commented 9 years ago

I replaced the view with in expressView and I got periods working

var ExpressView = util.safeRequire('express/lib/view');

function View(name, options) {
  options = options || {};
  this.name = name;
  this.root = options.root;
  var engines = options.engines;
  this.defaultEngine = options.defaultEngine;
  //var ext = this.ext = extname(name);
  var ext = this.ext = "." + this.defaultEngine //.jsx
  //if (!ext && !this.defaultEngine) throw new Error('No default engine was specified and no extension was provided.');
  //if (!ext) name += (ext = this.ext = ('.' != this.defaultEngine[0] ? '.' : '') + this.defaultEngine);
  this.engine = engines[ext] || (engines[ext] = require(ext.slice(1)).__express);
  this.path = this.lookup(name);
}

View.prototype = Object.create(ExpressView.prototype)

This uses the .jsx rendering engine for everything, regardless if there is a period in the name.

reggi commented 9 years ago

React router takes routes with periods! YAY!

<Router.Route name='hello.world' handler={Index} />

screen shot 2015-07-10 at 2 27 53 pm

samsel commented 9 years ago

@reggi is this already taken care of? do you need help with anything?

reggi commented 9 years ago

@samsel I had some problems with running tests on the code above. I couldn't get it to work so I never put a pull request.

samsel commented 9 years ago

@reggi you can send a PR to the develop branch. I can help fix tests.

reggi commented 9 years ago

Please forgive the mess! The last referenced issue is my fork on the develop branch. Here's the PR https://github.com/paypal/react-engine/pull/58.

reggi commented 9 years ago

Hey @samsel I put in a new pull request I copied the latest express view and commented out one line of code. Here's my issue / request in express: https://github.com/strongloop/express/issues/2708#issuecomment-122002655

Alternatively periods can be stripped out and matched to the corresponding route.

app.use(function(req, res){
  var theUrl = url.parse(req.url).pathname.replace(/\.+/g, "")
  return res.render(theUrl)
})

In the case above /app, /a.p.p, /ap.p should all render the react route /app.

reggi commented 9 years ago

@samsel just to confirm react route doesn't change what's rendered based on the url query does it?

dglozic commented 9 years ago

We just hit this as well with a valid URL containing identifiers that have dots:

/a/path/to/resources/aaa.bbb.ccc.ddd

We have not control over the shape of the ID and it looks like a valid path segment anyway. Is the fix on the way?

reggi commented 9 years ago

I put a pull request that fixes this and it was never accepted. It augments the native express view to compensate for the periods. Express 5 may help the issue I opened an issue with express a while back.

Note: the main reason this issue happens is because express views / render takes a file name with extension and when it sees a period it tries to look for the corresponding view. When we pass a path into res.render instead of a file express thinks it's a file template not a URL path.

On Monday, October 5, 2015, dglozic notifications@github.com wrote:

We just hit this as well with a valid URL containing identifiers that have dots:

/a/path/to/resources/aaa.bbb.ccc.ddd

We have not control over the shape of the ID and it looks like a valid path segment anyway. Is the fix on the way?

— Reply to this email directly or view it on GitHub https://github.com/paypal/react-engine/issues/52#issuecomment-145727158.

dglozic commented 9 years ago

Yes, I crawled through the code and saw where it happens. Meanwhile, can react-engine at least catch the error and pass it up to express so that we can get it in this form:

res.render(path, settings, function(err, html) { ...

Right now, our code blows up because error is thrown in the view, not in the engine.

If we try/catch res.render, do you see something wrong with it? The code seems syncronous, so old school try/catch seems appropriate here. We would at least like to report an error rather than crash the app.

reggi commented 9 years ago

@dglozic Not sure about how to catch the error. The simple thing here is to check the path for periods.

I'm thinking of a piece of middleware that adds a new method to res that's a appropriate wrapper for res.render().

pradeepsng30 commented 6 years ago

is there any update on this issue? I see a PR https://github.com/paypal/react-engine/pull/58 as a fix for this issue which was closed. Is the fix for this issue expected?