-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix(5955): ensure getEnvironment() function to return the correct environment values #38614
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
Merged
+280
−13
Merged
Changes from 4 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
c36137b
fix(5955): ensure getEnvironment() function to return the correct env…
DDDDDanica ec732ff
Merge remote-tracking branch 'origin/main' into fix/5955-env-logic
DDDDDanica b1516e9
chore(5956): lint fix
DDDDDanica 6d9fdfb
chore(5956): fix Empty string GITHUB_HEAD_REF breaks branch detection
DDDDDanica 16cdd36
chore(5956): fix Falsy empty string causes incorrect environment fall…
DDDDDanica da8f9d0
chore(5956): throw error when getBuildTargetFromTask has no match
DDDDDanica 01644ef
chore(5956): guard type and remove undefine param
DDDDDanica b48d5c6
chore(5956): fix unit test
DDDDDanica ac66e77
Merge branch 'main' into fix/5955-env-logic
DDDDDanica 70b4725
Merge branch 'main' into fix/5955-env-logic
DDDDDanica 4f10319
chore(5956): fix remove the type assertion
DDDDDanica 3fadd50
Merge remote-tracking branch 'origin/main' into fix/5955-env-logic
DDDDDanica ba57673
chore(5956): fix Build fails for low-level tasks passed as entry
DDDDDanica 6c8e6d7
fix: rename metamaskEnvironment to targetEnvironment
DDDDDanica 29f6262
fix: make resolvedEnvironment mandatory
DDDDDanica 569936c
fix: Dry-run message shows incorrect auto-detection status
DDDDDanica 1fcca68
fix: expand SPECIAL_TASK_MAPPINGS for remaining standalone tasks
DDDDDanica 3dddcb9
chore(5956): add all task mappings to SPECIAL_TASK_MAPPINGS
DDDDDanica 05e582b
Merge branch 'main' into fix/5955-env-logic
DDDDDanica b0f9784
Merge branch 'main' into fix/5955-env-logic
DDDDDanica 2d31221
Merge branch 'main' into fix/5955-env-logic
DDDDDanica 86ad1cc
Merge branch 'main' into fix/5955-env-logic
DDDDDanica 60fb0ed
fix: remove incorrect changes, fix fragile getIgnoredFiles() logic
DDDDDanica 0ac4bd5
fix: fix lint and ts type
DDDDDanica File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ const { readFileSync, writeFileSync } = require('fs'); | |
| const semver = require('semver'); | ||
| const { capitalize } = require('lodash'); | ||
| const { loadBuildTypesConfig } = require('../lib/build-type'); | ||
| const { BUILD_TARGETS, ENVIRONMENT } = require('./constants'); | ||
| const { BUILD_TARGETS, ENVIRONMENT, TASK_PREFIXES } = require('./constants'); | ||
|
|
||
| /** | ||
| * Returns whether the current build is a development build or not. | ||
|
|
@@ -29,6 +29,43 @@ function isTestBuild(buildTarget) { | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Extract the actual build target from a task name. | ||
| * | ||
| * Task names follow patterns like: | ||
| * - 'scripts:core:dev:standardEntryPoints' -> 'dev' | ||
| * - 'scripts:core:test:contentscript' -> 'test' | ||
| * - 'scripts:core:test-live:sentry' -> 'testDev' | ||
| * - 'test' -> 'test' (already a build target) | ||
| * | ||
| * @param {string} taskName - The task name or build target. | ||
| * @returns {BUILD_TARGETS | string} The extracted build target. | ||
| */ | ||
| function getBuildTargetFromTask(taskName) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Child processes receive task names (e.g., |
||
| // If it's already a valid build target, return it | ||
| const validTargets = Object.values(BUILD_TARGETS); | ||
| if (validTargets.includes(taskName)) { | ||
| return taskName; | ||
| } | ||
|
|
||
| // Create reverse mapping from prefix to build target | ||
| // Sort by prefix length descending to ensure longer prefixes match first | ||
| // (e.g., 'scripts:core:test-live' before 'scripts:core:test') | ||
| const prefixEntries = Object.entries(TASK_PREFIXES).sort( | ||
| ([, prefixA], [, prefixB]) => prefixB.length - prefixA.length, | ||
| ); | ||
|
|
||
| for (const [buildTarget, prefix] of prefixEntries) { | ||
| if (taskName.startsWith(prefix)) { | ||
| return buildTarget; | ||
| } | ||
| } | ||
|
|
||
| // If no match found, return the original task name | ||
| // (getEnvironment will handle it appropriately) | ||
| return taskName; | ||
Gudahtt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
Gudahtt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Map the current version to a format that is compatible with each browser. | ||
| * | ||
|
|
@@ -114,12 +151,24 @@ Good luck on your endeavors.`, | |
| /** | ||
| * Get the environment of the current build. | ||
| * | ||
| * This is a pure function that determines the build environment based on the | ||
| * build target and optional git context. When git context is not provided, | ||
| * it falls back to reading from process.env for backwards compatibility. | ||
| * | ||
| * @param {object} options - Build options. | ||
| * @param {BUILD_TARGETS} options.buildTarget - The target of the current build. | ||
| * @param {{ branch?: string, eventName?: string }} [options.git] - Optional git context for pure function usage. | ||
| * @returns {ENVIRONMENT} The current build environment. | ||
| */ | ||
| function getEnvironment({ buildTarget }) { | ||
| const branch = process.env.GITHUB_HEAD_REF || process.env.GITHUB_REF_NAME; | ||
| function getEnvironment({ buildTarget, git }) { | ||
Gudahtt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Use provided git context or fall back to process.env for backwards compatibility | ||
| const branch = | ||
| git?.branch || | ||
| process.env.GITHUB_HEAD_REF || | ||
| process.env.GITHUB_REF_NAME || | ||
| ''; | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const eventName = git?.eventName || process.env.GITHUB_EVENT_NAME || ''; | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // get environment slug | ||
| if (buildTarget === BUILD_TARGETS.PROD) { | ||
| return ENVIRONMENT.PRODUCTION; | ||
|
|
@@ -131,7 +180,7 @@ function getEnvironment({ buildTarget }) { | |
| return ENVIRONMENT.RELEASE_CANDIDATE; | ||
| } else if (branch === 'main') { | ||
| return ENVIRONMENT.STAGING; | ||
| } else if (process.env.GITHUB_EVENT_NAME === 'pull_request') { | ||
| } else if (eventName === 'pull_request') { | ||
| return ENVIRONMENT.PULL_REQUEST; | ||
| } | ||
| return ENVIRONMENT.OTHER; | ||
|
|
@@ -299,6 +348,7 @@ function makeSelfInjecting(filePath) { | |
| module.exports = { | ||
| getBrowserVersionMap, | ||
| getBuildName, | ||
| getBuildTargetFromTask, | ||
| getEnvironment, | ||
| isDevBuild, | ||
| isTestBuild, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| import { getEnvironment, getBuildTargetFromTask } from './utils'; | ||
| import { TASKS } from './constants'; | ||
|
|
||
| describe('getBuildTargetFromTask', () => { | ||
| it('passes through valid build targets', () => { | ||
| expect(getBuildTargetFromTask('dev')).toBe('dev'); | ||
| expect(getBuildTargetFromTask('test')).toBe('test'); | ||
| expect(getBuildTargetFromTask('prod')).toBe('prod'); | ||
| expect(getBuildTargetFromTask('dist')).toBe('dist'); | ||
| expect(getBuildTargetFromTask('testDev')).toBe('testDev'); | ||
| }); | ||
|
|
||
| it('extracts build target from task names', () => { | ||
| expect( | ||
| getBuildTargetFromTask(TASKS.SCRIPTS_CORE_DEV_STANDARD_ENTRY_POINTS), | ||
| ).toBe('dev'); | ||
| expect( | ||
| getBuildTargetFromTask(TASKS.SCRIPTS_CORE_TEST_STANDARD_ENTRY_POINTS), | ||
| ).toBe('test'); | ||
| expect( | ||
| getBuildTargetFromTask(TASKS.SCRIPTS_CORE_PROD_STANDARD_ENTRY_POINTS), | ||
| ).toBe('prod'); | ||
| expect( | ||
| getBuildTargetFromTask(TASKS.SCRIPTS_CORE_DIST_STANDARD_ENTRY_POINTS), | ||
| ).toBe('dist'); | ||
| expect( | ||
| getBuildTargetFromTask( | ||
| TASKS.SCRIPTS_CORE_TEST_LIVE_STANDARD_ENTRY_POINTS, | ||
| ), | ||
| ).toBe('testDev'); | ||
| }); | ||
|
|
||
| it('returns unknown patterns unchanged', () => { | ||
| expect(getBuildTargetFromTask('unknown:task')).toBe('unknown:task'); | ||
| expect(getBuildTargetFromTask(TASKS.MANIFEST_DEV)).toBe(TASKS.MANIFEST_DEV); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getEnvironment', () => { | ||
| const { env } = process; | ||
|
|
||
| beforeEach(() => { | ||
| process.env = { ...env }; | ||
| // Clear GitHub env vars that CI sets | ||
| delete process.env.GITHUB_HEAD_REF; | ||
| delete process.env.GITHUB_REF_NAME; | ||
| delete process.env.GITHUB_EVENT_NAME; | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| process.env = env; | ||
| }); | ||
|
|
||
| describe('with explicit git context (pure function usage)', () => { | ||
| it('returns correct environment for build targets', () => { | ||
| const git = { branch: '', eventName: '' }; | ||
| expect(getEnvironment({ buildTarget: 'prod' as never, git })).toBe( | ||
| 'production', | ||
| ); | ||
| expect(getEnvironment({ buildTarget: 'dev' as never, git })).toBe( | ||
| 'development', | ||
| ); | ||
| expect(getEnvironment({ buildTarget: 'test' as never, git })).toBe( | ||
| 'testing', | ||
| ); | ||
| expect(getEnvironment({ buildTarget: 'testDev' as never, git })).toBe( | ||
| 'development', | ||
| ); | ||
| expect(getEnvironment({ buildTarget: 'dist' as never, git })).toBe( | ||
| 'other', | ||
| ); | ||
| }); | ||
|
|
||
| it('returns RELEASE_CANDIDATE for dist on release branch', () => { | ||
| const git = { branch: 'release/13.12.0', eventName: '' }; | ||
| expect(getEnvironment({ buildTarget: 'dist' as never, git })).toBe( | ||
| 'release-candidate', | ||
| ); | ||
| }); | ||
|
|
||
| it('returns STAGING for dist on main branch', () => { | ||
| const git = { branch: 'main', eventName: '' }; | ||
| expect(getEnvironment({ buildTarget: 'dist' as never, git })).toBe( | ||
| 'staging', | ||
| ); | ||
| }); | ||
|
|
||
| it('returns PULL_REQUEST for dist on pull_request event', () => { | ||
| const git = { branch: '', eventName: 'pull_request' }; | ||
| expect(getEnvironment({ buildTarget: 'dist' as never, git })).toBe( | ||
| 'pull-request', | ||
| ); | ||
| }); | ||
|
|
||
| it('returns TESTING for test even with pull_request event', () => { | ||
| const git = { branch: '', eventName: 'pull_request' }; | ||
| expect(getEnvironment({ buildTarget: 'test' as never, git })).toBe( | ||
| 'testing', | ||
| ); | ||
| }); | ||
|
|
||
| it('prioritizes release branch over pull_request event', () => { | ||
| const git = { branch: 'release/14.0.0', eventName: 'pull_request' }; | ||
| expect(getEnvironment({ buildTarget: 'dist' as never, git })).toBe( | ||
| 'release-candidate', | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('with process.env fallback (backwards compatibility)', () => { | ||
| it('returns correct environment for build targets', () => { | ||
| expect(getEnvironment({ buildTarget: 'prod' as never })).toBe( | ||
| 'production', | ||
| ); | ||
| expect(getEnvironment({ buildTarget: 'dev' as never })).toBe( | ||
| 'development', | ||
| ); | ||
| expect(getEnvironment({ buildTarget: 'test' as never })).toBe('testing'); | ||
| expect(getEnvironment({ buildTarget: 'testDev' as never })).toBe( | ||
| 'development', | ||
| ); | ||
| expect(getEnvironment({ buildTarget: 'dist' as never })).toBe('other'); | ||
| }); | ||
|
|
||
| it('returns RELEASE_CANDIDATE for dist on release branch', () => { | ||
| process.env.GITHUB_REF_NAME = 'release/13.12.0'; | ||
| expect(getEnvironment({ buildTarget: 'dist' as never })).toBe( | ||
| 'release-candidate', | ||
| ); | ||
| }); | ||
|
|
||
| it('returns STAGING for dist on main branch', () => { | ||
| process.env.GITHUB_REF_NAME = 'main'; | ||
| expect(getEnvironment({ buildTarget: 'dist' as never })).toBe('staging'); | ||
| }); | ||
|
|
||
| it('returns PULL_REQUEST for dist on pull_request event', () => { | ||
| process.env.GITHUB_EVENT_NAME = 'pull_request'; | ||
| expect(getEnvironment({ buildTarget: 'dist' as never })).toBe( | ||
| 'pull-request', | ||
| ); | ||
| }); | ||
|
|
||
| it('returns TESTING for test even with pull_request event', () => { | ||
| process.env.GITHUB_EVENT_NAME = 'pull_request'; | ||
| expect(getEnvironment({ buildTarget: 'test' as never })).toBe('testing'); | ||
| }); | ||
|
|
||
| it('prefers GITHUB_HEAD_REF over GITHUB_REF_NAME', () => { | ||
| process.env.GITHUB_HEAD_REF = 'release/15.0.0'; | ||
| process.env.GITHUB_REF_NAME = 'main'; | ||
| expect(getEnvironment({ buildTarget: 'dist' as never })).toBe( | ||
| 'release-candidate', | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.