-
Notifications
You must be signed in to change notification settings - Fork 929
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
fix: identify dependencies that might require the --no-tree-shake-icons flag #7724
base: master
Are you sure you want to change the base?
Conversation
…flutter-tree-shake
…-tools into fix/flutter-tree-shake
src/frameworks/flutter/utils.ts
Outdated
const pubSpecPath = "./pubspec.yaml"; | ||
let pubSpec: Record<string, any> = {}; | ||
try { | ||
const pubSpecBuffer = await readFile(pubSpecPath); |
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.
we should use a path relative to the project root as the project can be located in a subdir, as we do in discover
firebase-tools/src/frameworks/flutter/index.ts
Lines 16 to 20 in cc6744f
export async function discover(dir: string): Promise<Discovery | undefined> { | |
if (!(await pathExists(join(dir, "pubspec.yaml")))) return; | |
if (!(await pathExists(join(dir, "web")))) return; | |
const pubSpecBuffer = await readFile(join(dir, "pubspec.yaml")); | |
const pubSpec = yaml.parse(pubSpecBuffer.toString()); |
src/frameworks/flutter/utils.ts
Outdated
@@ -8,3 +10,27 @@ export function assertFlutterCliExists() { | |||
"Flutter CLI not found, follow the instructions here https://docs.flutter.dev/get-started/install before trying again.", | |||
); | |||
} | |||
|
|||
export async function getTreeShakeFlag(): Promise<string> { |
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 we can make it generic like getAdditionalBuildArgs
and make it return an array of strings, then we can accommodate more use cases in the future if needed
Description
Related to this issue: #6197
Trying to identify dependencies that might require the --no-tree-shake-icons build flag.
Unfortunately can not allow users to pass arbitrary/custom flags when running the
deploy
function, so tried to identify dependencies that are likely to require the flag to build.Scenarios Tested
fixes #6197