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

Make compilation errors available to extensions. #33

Merged
merged 1 commit into from Aug 1, 2015

Conversation

@jagoda
Copy link
Contributor

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 #10.
@jagoda jagoda added the bug label Jul 11, 2015
@jagoda jagoda added this to the 2.0.2 milestone Jul 11, 2015
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Jul 14, 2015

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

@jagoda

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
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 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
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jagoda jagoda deleted the jagoda:fix-error-handling branch Aug 1, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Aug 8, 2015

The name _compileAll() is confusing to me.

@jagoda

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Member

hueniverse commented Aug 10, 2015

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

@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Aug 10, 2015

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

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Aug 10, 2015

Sure.

jagoda added a commit to jagoda/vision that referenced this pull request Aug 10, 2015
@jagoda

This comment has been minimized.

Copy link
Contributor Author

jagoda commented Aug 10, 2015

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.