fix: remove unintentionally spammy loadEnvironment warnings #2466
Conversation
|
|
Performance Test Results The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate. https://pr-2466.pwa-venia.com : LH Performance Expected 0.85 Actual 0.54, LH Best Practices Expected 1 Actual 0.92 |
revanth0212
changed the base branch from
revanth/buildpack_braintree_token_check
to
develop
zetlen
force-pushed
the
zetlen/remove-env-warning-spam
branch
from
5cb2e92
to
f4dc871
Compare
m2-community-project
bot
moved this from Ready for Review
to Review in Progress
in Pull Request Progress
| const buildpackVersion = require('../../package.json').version; | ||
|
|
||
| class CompatEnvAdapter { | ||
| static isExpired({ dateChanged, warnForDays }) { |
There was a problem hiding this comment.
Quick question JZ. Why did you make this a static method instead of a function outside the class? Not that it would make any difference, just to pick your brain.
There was a problem hiding this comment.
It's a good question though. Two reasons:
- It's a pure function that relies on no instance variables, BUT it's only used by this class
- I needed to use it in tests
| "reason": "The new environment variables DEV_SERVER_HOST and STAGING_SERVER_HOST now provide the host override functionality for the Docker setup." | ||
| } | ||
| ] | ||
| "changes": [] |
There was a problem hiding this comment.
Now that we have the EnvAdapter class looking at the expiry date and frequency, how about a sample change for that? Maybe even the brain tree token in question, maybe remind it every 30 days.
There was a problem hiding this comment.
It's actually not a frequency measure! It's a time duration during which it would warn every time you run the command.
What if we used the unit test to demonstrate that instead of adding a change here that we don't need?
…zetlen/remove-env-warning-spam
| this.UNSET = Symbol('FLAG_FOR_UNSET'); | ||
| this._actual = actual; | ||
| this._stored = new Map(); | ||
| afterEach(() => this._unmock()); |
There was a problem hiding this comment.
Had no idea we can do this, invoking jest helpers inside custom code.
m2-community-project
bot
moved this from Review in Progress
to Reviewer Approved
in Pull Request Progress
zetlen
added
the
version: Minor
m2-community-project
bot
moved this from Reviewer Approved
to Done
in Pull Request Progress
Description
Simplify the breaking change warnings in
loadEnvironment, and add a fail-safe to make sure they don't log forever.Changes:
defaultChangedandexampleChangedwarning types, because they're not useful and I was wrong to create them. When a default changes, you either don't care or you've overridden it. When an example changes, you really never care.Additions:
envVarDefinitionsthemselves, to ensure they're compliant.Related Issue
Fixes PWA-502
Fixes underlying problem from #2308
Acceptance
Verification Stakeholders
@brendanfalkowski @revanth0212
Specification
Verification Steps
Screenshots / Screen Captures (if appropriate)
Checklist