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

Support live reload and add optional debug log #158

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

jelhan
Copy link
Collaborator

@jelhan jelhan commented Nov 15, 2020

Ember CLI Content Security Policy had cached its configuration. This broke live reload because the cache was not invalidated when the addon's configuration file changed. This was reported in #146. Beside causing this bug the caching also added a lot of complexity.

This pull request removes the caching all together. Instead the configuration is calculated whenever it's needed. This has a negative impact on performance. Especially because configuration file is read multiple times - even for the same build. But I don't think the performance impact is noticeable by end-user because the file is still read less than 10 times, it's very small and filesystem caching should speed up subsequent reads.

It also avoid adding report-uri to a policy, which is delivered via meta tag. This addresses #148 at least partly.

Additionally it introduces a debug output using debug NPM package. User can opt-in to debug output by setting DEBUG environment variable to ember-cli-content-security-policy, e.g. DEBUG=ember-cli-content-security-policy ember build.

The debug package is used in ember-auto-import, embroider and ember-data. It seems to be a well established tool in Ember ecosystem.

Closes #146

@jelhan jelhan added the bug label Nov 15, 2020
@jelhan jelhan changed the title Support live reload Support live reload and add optional debug log Nov 15, 2020
Comment on lines 65 to 67
afterEach(async function() {
await removeConfig(testProject);
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global afterEach caused issues because it was running after every test even if part of a describe hook. Some describe blocks are running multiple tests against the same running development server. After fixing live reload support this showed strange timing issues. Moved all tests into describe hooks and avoided global afterEach to have a more fine granular control.

Comment on lines 65 to 129
describe('causes tests to fail on CSP violations', async function() {
afterEach(async function() {
await removeConfig(testProject);
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global afterEach caused issues because it was running after every test even if part of a describe hook. Some describe blocks are running multiple tests against the same running development server. After fixing live reload support this showed strange timing issues. Moved all tests into describe hooks and avoided global afterEach to have a more fine granular control.

Comment on lines 83 to 190
it('does not cause tests failures if addon is disabled', async function() {
await setConfig(testProject, {
enabled: false,
});
let { exitCode } = await testProject.runEmberCommand('test');

expect(exitCode).to.equal(0);
});

it('does not cause tests failures if `failTests` config option is `false`', async function() {
await setConfig(testProject, {
failTests: false,
});

let { exitCode } = await testProject.runEmberCommand('test');

expect(exitCode).to.equal(0);
});

// One common scenario is when running the server in production mode locally, i.e.
// you are doing the production build on your local machine, connecting to your actual production server,
// for example via `ember serve -prod`. In these cases we don't want this addon to break.
it('does not break development server for builds not including tests', async function() {
// TODO: not supported yet
await testProject.startEmberServer({
environment: 'prodocution',
port: '49741',
});

let response = await request({
url: 'http://localhost:49741/',
headers: {
'Accept': 'text/html'
}
});
let responseForTests = await request({
url: 'http://localhost:49741/tests',
headers: {
'Accept': 'text/html'
}
});

expect(response.statusCode).to.equal(200);
expect(responseForTests.statusCode).to.equal(200);

await testProject.stopEmberServer();
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are just moved from another position within the file. Beside that they are untouched if I recall correctly.

const { project } = this;
const { ui } = project;
const ownConfig = readConfig(project, environment);
const runConfig = this._runConfig;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't researched if the run-time configuration is available through some public API on the project or app. I'm not a big fan of storing it in config hook and using it later. But I didn't wanted to touch to many parts at the same time.

Copy link
Collaborator

@sandstrom sandstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@jelhan
Copy link
Collaborator Author

jelhan commented Nov 23, 2020

Pulled out the changes of the tests in #163 and #164, resolved merge conflicts caused by adding Prettier in #162 and rebased against latest master. I hope that tests are passing due to #161. Let's see.

@jelhan jelhan merged commit 2e2e725 into master Nov 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the do-not-cache-configuration branch November 23, 2020 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Development server must be restarted after each configuration change
2 participants