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

fix livereload #105

Merged
merged 1 commit into from
Aug 8, 2019
Merged

Conversation

jelhan
Copy link
Collaborator

@jelhan jelhan commented Aug 5, 2019

Allowing connections and script source required by livereload feature was broken by configuration refactoring. This fixes it and adds tests.

This PR is based on #104. It should be merged afterwards.

@rwjblue
Copy link
Member

rwjblue commented Aug 6, 2019

Ready for a rebase now that #104 has landed

@jelhan jelhan changed the title WIP: fix livereload fix livereload Aug 6, 2019
@jelhan
Copy link
Collaborator Author

jelhan commented Aug 6, 2019

Rebased and fixed the test that were failing due to leaking state.

@jelhan jelhan changed the title fix livereload WIP: fix livereload Aug 6, 2019
@jelhan
Copy link
Collaborator Author

jelhan commented Aug 6, 2019

This is not ready yet. This had moved the calculation of config to serverMiddleware hook but that's only executed on ember serve but not on ember test and ember build. Will need to refactor to calculate the config in a more common hook (e.g. included as before) and handle the livereload configuration separately.

@jelhan
Copy link
Collaborator Author

jelhan commented Aug 7, 2019

Pushed a new implementation. It's a total rewrite of this PR. I guess a new review + approval is required.

It does not treat live reload options as part of config anymore. They are calculated independently from other config in serverMiddleware hook and cached for contenFor hook. Live reload should be disabled if serverMiddleware hook is not executed cause live reload depends on express server. Luckily contentFor hook is executed after serverMiddleware hook.

Before my rewrites of all that stuff live reload was calculated in contentFor and serverMiddleware hooks independently:
https://github.com/rwjblue/ember-cli-content-security-policy/blob/4aabfa50496c720a76fa1e689ea43c04a40be875/index.js#L122-L129
https://github.com/rwjblue/ember-cli-content-security-policy/blob/4aabfa50496c720a76fa1e689ea43c04a40be875/index.js#L173-L184

This introduces inconsistency between both which may result in broken live reload support if CSP is put in meta tag. Especially the contentFor hook ignored the live reload host option cause that value wasn't available trough environment variables.

@jelhan jelhan changed the title WIP: fix livereload fix livereload Aug 7, 2019
@jelhan
Copy link
Collaborator Author

jelhan commented Aug 7, 2019

Squashed commits.

@rwjblue rwjblue merged commit fbddcc6 into adopted-ember-addons:master Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants