Skip to content

Make compilation errors available to extensions.#33

Merged
jagoda merged 1 commit intohapijs:masterfrom
jagoda:fix-error-handling
Aug 1, 2015
Merged

Make compilation errors available to extensions.#33
jagoda merged 1 commit intohapijs:masterfrom
jagoda:fix-error-handling

Conversation

@jagoda
Copy link
Copy Markdown
Contributor

@jagoda jagoda commented Jul 11, 2015

Currently neither template compilation nor rendering occur until
after the 'onPreResponse' event. This means that extensions have
no way to intercept template related errors. This change moves the
compilation step to an earlier point in the request lifecycle so
that compilation errors are made available to the 'onPostHandler'
and 'onPreResponse' extension points. Rendering still happens
after 'onPreResponse' so that the context object can be modified
by extensions all the way up through the 'onPreResponse' event.

Fixes #10.

Currently neither template compilation nor rendering occur until
after the 'onPreResponse' event. This means that extensions have
no way to intercept template related errors. This change moves the
compilation step to an earlier point in the request lifecycle so
that compilation errors are made available to the 'onPostHandler'
and 'onPreResponse' extension points. Rendering still happens
after 'onPreResponse' so that the context object can be modified
by extensions all the up through the 'onPreResponse' event.

Fixes hapijs#10.
@jagoda jagoda added the bug Bug or defect label Jul 11, 2015
@jagoda jagoda added this to the 2.0.2 milestone Jul 11, 2015
@jagoda
Copy link
Copy Markdown
Contributor Author

jagoda commented Jul 14, 2015

@arb any chance you have some time to look this over?

@jagoda
Copy link
Copy Markdown
Contributor Author

jagoda commented Jul 20, 2015

I would appreciate it if somebody else from @hapijs/core-project-maintainers could review this before it is merged since it is a non-trivial change.

@arb
Copy link
Copy Markdown
Contributor

arb commented Jul 20, 2015

Looking at the scope of this change, I think you should bump the major version to minimize any potential issues. While the rewrite is to fix what we're calling a bug, this is a lot of code moving around. All the unit tests may pass, but we don't know how this plugin is being used "out there".

As far as actual implementation goes, I'm not familiar enough with this plugin to speak intelligently about it.

@jagoda
Copy link
Copy Markdown
Contributor Author

jagoda commented Jul 20, 2015

Makes sense. Thanks for the feedback.

@jagoda jagoda modified the milestones: 3.0.0, 2.0.2 Jul 20, 2015
@jagoda
Copy link
Copy Markdown
Contributor Author

jagoda commented Jul 22, 2015

@hueniverse it seems that you may be the only other person familiar with this code. I'd welcome a review from you if you have time. Otherwise, I'm comfortable merging this as is.

@jagoda jagoda added the breaking changes Change that can breaking existing code label Jul 28, 2015
@jagoda jagoda self-assigned this Jul 30, 2015
jagoda added a commit that referenced this pull request Aug 1, 2015
Make compilation errors available to extensions.
@jagoda jagoda merged commit d48abeb into hapijs:master Aug 1, 2015
@jagoda jagoda deleted the fix-error-handling branch August 1, 2015 14:06
@hueniverse
Copy link
Copy Markdown
Contributor

The name _compileAll() is confusing to me.

@jagoda
Copy link
Copy Markdown
Contributor Author

jagoda commented Aug 10, 2015

Fair enough. I tried to think of a different name but didn't manage to come up with one that I liked better. Do you have something in mind that you think works better? I'm happy to change it.

@hueniverse
Copy link
Copy Markdown
Contributor

I tend to pick names that describe the stages (prepare, finalize, etc.)

@jagoda
Copy link
Copy Markdown
Contributor Author

jagoda commented Aug 10, 2015

Cool, that makes sense. So would _prepare() make sense to you?

@hueniverse
Copy link
Copy Markdown
Contributor

Sure.

jagoda added a commit to jagoda/vision that referenced this pull request Aug 10, 2015
@jagoda
Copy link
Copy Markdown
Contributor Author

jagoda commented Aug 10, 2015

Done.

@lock
Copy link
Copy Markdown

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking changes Change that can breaking existing code bug Bug or defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Verify template and layout files exist in handler

3 participants