Fastboot instance initializer throws if reportOnly config is false #271
Conversation
The current behavior makes it impossible to disable reportOnly mode, we actually want to ensure the configuration is properly defined, not that it's true.
| addonConfig && | ||
| Object.prototype.hasOwnProperty.call(addonConfig, 'reportOnly') && | ||
| Object.prototype.hasOwnProperty.call(addonConfig, 'policy') |
There was a problem hiding this comment.
Wondering if we could assert against the type instead?
| addonConfig && | |
| Object.prototype.hasOwnProperty.call(addonConfig, 'reportOnly') && | |
| Object.prototype.hasOwnProperty.call(addonConfig, 'policy') | |
| typeof addonConfig === 'object' && | |
| typeof addonConfig.reportOnly === 'boolean' && | |
| typeof addonConfig.policy === 'string' |
This seems to be more readable and even make the assertion more strict.
There was a problem hiding this comment.
I can't seem to find an existing test that actually runs goes through that codepath. Any ideas?
I'm not too experienced with Ember, I have no clue where to begin to setup tests deeply integrated with Fastboot like that.
I can apply your suggestion, but I believe policy should be 'object' right?
There was a problem hiding this comment.
I changed the implementation as suggested, I will not have time to add tests for this, sorry. Hopefully, this can still be merged and released, as currently, v2.0.0 crashes on startup when reportOnly is set to false in a dev environment.
We're using our fork for now.
There was a problem hiding this comment.
The tests are failing with latest changes. Maybe my suggestion isn't working as expected.
Not sure when I have time to look into it. No one is paying me for maintaining this addon. I'm doing it _after_y regular job.
If you have time to debug that would be great.
For tests please have a look at existing Fastboot related tests in
node-testsfolder.
I added a test. It doesn't actually fail with the previous code, since assert only fails in development mode, but as I said I have no clue how to make the test server behave like a dev environment would.
The failing tests were caused by me using object for policy, I went back to string and it works well now.
There was a problem hiding this comment.
@jelhan Just making sure you've seen this notification. No worries if you didn't have time to get to this yet!
|
Please ignore the failing test for Ember CLI beta. It is not related to your changes. It should be fixed by #272. |
|
The tests are failing with latest changes. Maybe my suggestion isn't working as expected. Not sure when I have time to look into it. No one is paying me for maintaining this addon. I'm doing it _after_y regular job. If you have time to debug that would be great. For tests please have a look at existing Fastboot related tests in |
| await removeConfig(testProject); | ||
| }); | ||
|
|
||
| it.only('sets CSP header if served via FastBoot', async function () { |
There was a problem hiding this comment.
This seems to be a left-over from debugging.
| it.only('sets CSP header if served via FastBoot', async function () { | |
| it('sets CSP header if served via FastBoot', async function () { |
@jelhan Done! That makes perfect sense :) |
jelhan
changed the title
|
Released this as v2.0.2 just right now. I'm sorry that it took that long. To be honest I just missed that I hadn't released it yet. |
The current behaviour makes it impossible to disable
reportOnlymode, we actually want to ensure the configuration is properly defined, not that it's true.