Skip to content
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 uploadToS3 import path #5472

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix uploadToS3 import path #5472

wants to merge 2 commits into from

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Jan 9, 2025

This potentially fixes MNTOR-3922 as well, since the icon upload imported from the .js file:

import { uploadToS3 } from "../../utils/s3.js";

I reverted s3.ts back to a JS file because it was still being imported from the JS upload script. Or at least, it was supposed to, as that had the wrong path.

Vinnl added 2 commits January 9, 2025 16:08
This reverts commit e4573d5.

It's still being imported from
src/scripts/build/uploadAutoCompleteLocations.js, so it can't be
plain TypeScript yet.
Also change back to a regular import; I don't think it does much
harm to import it when unnecessary, and potentially obfuscates
invalid import paths.
@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Jan 9, 2025
@Vinnl Vinnl requested a review from rhelmer January 9, 2025 15:16
@Vinnl Vinnl self-assigned this Jan 9, 2025
@@ -27,6 +27,7 @@ import os from "os";
import path from "path";
import fs from "fs";
import AdmZip from "adm-zip";
import { uploadToS3 } from "../../utils/s3.js";
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the changed path - but also, @rhelmer, you explicitly made this into a dynamic import here:

https://github.com/mozilla/blurts-server/pull/4949/files#diff-86136336e578d7cf634398ffcbdfaf05b5b938e85866325dad589fb4ff98b191R331

Was that just to avoid importing it if not used? Because I think that potentially obscures the path being invalid, so I changed it to a regular import - but let me know if there's another reason.

Copy link

github-actions bot commented Jan 9, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant