-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[fingerprint] support skip app versions from app config #28712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes:
- Need to think through how people will configure this who don't have access to the JS API (a file similar to
.fingerprintignore
maybe? how will the two interact?) - If we do go with the skips pattern, I think some of the items should probably be separated into their own skip enum option.
@@ -74,6 +74,8 @@ export const DEFAULT_IGNORE_PATHS = [ | |||
].join(',')}}/**/*`, | |||
]; | |||
|
|||
const DEFAULT_SOURCE_SKIPS = SourceSkips.AppConfigVersion | SourceSkips.AppConfigRuntimeVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be best to have None be the default? I realize runtime version is the logical default currently. I'm just more thinking about the different uses for this library and how to reduce breaking changes going forward (where a breaking change is a change that produces a fingerprint difference without changing the user-specified config):
- As a standalone library, just running fingerprint from
npx @expo/fingerprint
. In this case it maybe makes sense to have some defaults, though I'm not sure. I think that configuration should probably mostly done through.fingerprintignore
and another analogous concept for source skips. - As a part of the fingerprint runtimeVersion policy. In this case, all configuration needs to be done via
.fingerprintignore
and another analogous concept for source skips. We were okay with runtime version skip being default for it since it uses the policy and happened to not overlap with the skip. But going forward, I think None should be the default for this policy. We should strive to not introduce breaking changes here, though it's not the end of the world since the version of@expo/fingerprint
used is tied to the expo-updates version which upgrading would trigger a new fingerprint anyways. - Through the JS API. In this case, I think having None be default makes the most sense since it's so easy for people to specify.
So overall, I think what we need is a way for people to define this in their project similar to .fingerprintignore
. And maybe come up with a story of how the two interact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i generally agree with @wschurman that we probably want to keep this list pretty small - although, i think we could get away with using the app version by default here, and i wouldn't be opposed to shipping that as-is. i'll leave that up to you guys to decide.
for anything beyond that, an approach for controlling ignores via something like .fingerprintignore (such as fingerprint.config.js) would be great - @Kudo brought this up on our sync today and i think we're on the same page. we could express our defaults in a version of that file that we create for people when they init fingerprint in their project.
AppConfigRuntimeVersion = 1 << 2, | ||
|
||
// Skip app name in app.json | ||
AppConfigName = 1 << 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AppConfigName = 1 << 3, | |
AppConfigNameDescriptionShortName = 1 << 3, |
(thinking more, maybe these should be separate skips?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be basically used for app repacking and i don't want to introduce too many options. let's use ExpoConfigNames
for that.
|
||
if (sourceSkips & SourceSkips.AppConfigSchemes) { | ||
delete normalizedConfig.scheme; | ||
normalizedConfig.slug = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is slug part of schemes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
during prebuild we will actually generate an implicit scheme based on slug.
# Why follow up feedback from #28712 that we should add fingerprint config support. # How introduce `fingerprint.config.js` support to override Options. not all options are supported, because `ignorePaths` we would like to use `.fingerprintignore` and `platform` is not supported for sure. the priority is explicit Options > fingerprint.config.js > default Options.
Co-authored-by: Will Schurman <[email protected]>
67e0a99
to
253de74
Compare
update the pr based on the feedback
|
Why
i've heard different feedback that they would like to ignore fingerprinting
version
from app.json. in their workflow they would bump version in app.json for simply for updates.references:
How
sourceSkips
bitwise flags that would potentially support more aggresive fingerprint for app repacking.currently by default the sourceSkips isUpdates: change toconst DEFAULT_SOURCE_SKIPS = SourceSkips.AppConfigVersion | SourceSkips.AppConfigRuntimeVersion;
SourceSkips.None
Test Plan
add some unit tests
Checklist
npx expo prebuild
& EAS Build (eg: updated a module plugin).