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

Fixed issue https://github.com/mde/ejs/issues/119 #129

Merged
merged 6 commits into from
Apr 18, 2016
Merged

Fixed issue https://github.com/mde/ejs/issues/119 #129

merged 6 commits into from
Apr 18, 2016

Conversation

dgofman
Copy link
Contributor

@dgofman dgofman commented Feb 1, 2016

Run test script by command

npm run-script sample

Open browser and goto http://localhost:3000/

Before

 ReferenceError: ./ejs/test/sample/index.html:2
   1| <html> 
>> 2| <head> 
   3| <% include header.html %> 
   4| </head> 
   5| <body>

After

ReferenceError: ./ejs/test/sample/body.html:2
    1| <!-- Execute an exception -->
 >> 2| <p><%= this_variable_is_not_defined %></p>

@dgofman dgofman mentioned this pull request Feb 1, 2016
Closed
@RyanZim RyanZim mentioned this pull request Mar 25, 2016
Closed
@RyanZim
Copy link
Collaborator

RyanZim commented Mar 25, 2016

@mde: I tried this and it does output better errors.
Would fix issues #118 & #119 if I'm correct.

Edit: This only partially solves these issues. It's a step in the right direction though.

Output for rendering index.ejs with an undefined var (namer) in a template (body.ejs) before this patch:

{ [ReferenceError: index.ejs:3
    1| <html>
    2|     <body>
 >> 3|         <% include body.ejs %>
    4|     </body>
    5| </html>

namer is not defined] path: 'index.ejs' }

Output after this patch:

{ [ReferenceError: /absolute/path/to/template/body.ejs:3
    1| <p><%= namer+" Zimmerman" %></p>

namer is not defined] path: '/absolute/path/to/template/body.ejs' }

I'm not sure that we want the sample folder & script included in the PR; especially since the script installs express.js (which is unnecessary since the errors are outputted to the command line).
The sample folder is 4.8KB; but after running npm run-script sample, node_modules gains about 1 MB due to express.js.
Maybe I'm missing your point for including them, @dgofman.

Also, perhaps the commits should be interactively rebased to tidy things up a bit before merging to master (@dgofman, don't rebase until @mde gives the OK).

Other than that 👍 to @dgofman IMO.

@RyanZim RyanZim mentioned this pull request Mar 25, 2016
Closed
@dgofman
Copy link
Contributor Author

dgofman commented Mar 25, 2016

My changes related only problems in HTML templates it's not cover all issues in EJS exceptions.
Whole idea to provide at least correct file name by using "include". My index html templates including 5-10 additional template file and if some error in any of them I am getting in exception log only the main file. And it's taking many time to find where is the issue.
Edit by @RyanZim: changed @include to include as it was referencing a user.

@RyanZim
Copy link
Collaborator

RyanZim commented Mar 25, 2016

Thanks for the clarification @dgofman.

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 7, 2016

@mde ping?

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 16, 2016 •
edited
Loading

On second thought, perhaps the sample folder could be used for testing.

@TimothyGu, could you look over this to make sure I'm not missing anything?

@dgofman
Copy link
Contributor Author

dgofman commented Apr 16, 2016

@RyanZim
I added this line only for testing.
It will not impact to any existing code

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 16, 2016

@dgofman I removed the line note for the time being. I'm not sure if we want to include the sample/ folder at all.

@dgofman
Copy link
Contributor Author

dgofman commented Apr 17, 2016

@RyanZim
Sample just for prove of fix we can remove completely

@mde
Copy link
Owner

mde commented Apr 18, 2016

LGTM. Thanks!

@mde mde merged commit 2b388ca into mde:master Apr 18, 2016
@RyanZim
Copy link
Collaborator

RyanZim commented Apr 18, 2016

@mde: Do you want the sample folder & script removed or not?

@dgofman
Copy link
Contributor Author

dgofman commented Apr 18, 2016 •
edited by RyanZim
Loading

Hi Matthew (@mde),

When are you planing to push new version ejs to NPM?

On Mon, Apr 18, 2016 at 9:28 AM, Matthew Eernisse notifications@github.com
wrote:

Merged #129 #129.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#129 (comment)

@Spikef Spikef mentioned this pull request May 28, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dgofman @RyanZim @mde