Replace term whitelist with allowlist #817
Conversation
|
Thank you so much! Sorry I must have missed your previous comment. Let me know if I can help with anything and feel free to close the previous PR #806 |
|
Is this in response to the Replace terms blacklist & whitelist in Ember CLI RFC? If so, I thought the settled upon terms were |
There was a problem hiding this comment.
First of all, thanks a million for doing all this work 🎉 this is amazing and really detailed!!
I'm looking at this and I can see that it's clearly a breaking change 🤔 I'm wondering if it would be better to implement some sort of deprecation around hostWhitelist so that we don't need to release this as a breaking change and we can give people a bit of time to update their system. What do you think?
|
fastboot has already released v3, as a minor bump we can have an alias for the old name (with deprecation would be even better); for ember-cli-fastboot it seems fine to include a breaking change, as the public interface in config is straight forward, i.e., |
|
@xg-wang I would feel quite strongly against releasing a breaking change for this in a major version that has never been deprecated once since that's not how we usually do things in Ember. I think it's totally fine for the alias and the deprecation to live for the entirety of the 3.x series 👍 especially since it won't be a massive code-path that needs to still hang around, as we can use the new name internally and just make sure that we are reading from the old name as a fallback |
kiwiupover
force-pushed
the
change-whitelist-to-allow-list
branch
from
99df2e5
to
48e44c6
Compare
kiwiupover
force-pushed
the
change-whitelist-to-allow-list
branch
from
48e44c6
to
d61b516
Compare
|
@mansona I have fixed the requested changes for this PR. Can you please review again. |
There was a problem hiding this comment.
This looks like a great PR! Are we concerned about making this breaking change while still in an alpha/beta 3.0? Especially with acceptable fallbacks and warnings to the end user?
As long as we agree on the approach of the PR, my preference is to move this forward given this is well documented (e.g. moduleAllowlist perhaps could be added to the README; CHANGELOG would need to clearly call this out).
README.md
Outdated
| @@ -273,23 +273,23 @@ module.exports = function(environment) { | |||
| }, | |||
|
|
|||
| fastboot: { | |||
| hostWhitelist: ['example.com', 'subdomain.example.com', /^localhost:\d+$/] | |||
| hostAllowlist: ['example.com', 'subdomain.example.com', /^localhost:\d+$/] | |||
There was a problem hiding this comment.
You might have a pending commit to standardize the casing
There was a problem hiding this comment.
I wouldn't be opposed to just making this Allowlist (lower case l) to ensure ease of migration
| @@ -83,15 +83,15 @@ function loadConfig(distPath) { | |||
| } | |||
|
|
|||
| let sandboxRequire = buildWhitelistedRequire( | |||
| pkg.fastboot.moduleWhitelist, | |||
| pkg.fastboot.moduleAllowlist, | |||
There was a problem hiding this comment.
I think we might want to add a fallback + deprecation here as well?
kiwiupover
force-pushed
the
change-whitelist-to-allow-list
branch
from
fe67e67
to
c939b82
Compare
I followed the RFC from awhile back that has since been merged, so
hopefully this can be changed here in ember-cli-fastboot as well.
This PR pushes @mcfiredrill work over the line #806
Closes #806