-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/#575 스크립트를 로드하는 유틸 함수 구현 #576
base: main
Are you sure you want to change the base?
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.
loadScript
를 유틸로 빼어낸 거 너무 좋은 것 같아요! 👍
다만 util에서 사용하려면, 디테일에서 조금 더 신경을 써주어야할 것 같습니다. 우선 제가 봤을 때 고쳐야할 곳에 코멘트 달아두었어요.
script.async = true; | ||
script.defer = true; |
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.
script.async = true;
만 있어도 될 것 같아요. (참고)
script.onload = () => resolve(); | ||
|
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.
script.onload = () => resolve(); | |
script.onerror = (error) => reject(error); |
script 로드에 실패했을 때 에러 처리가 필요해 보입니다.
const element = document.querySelector(`script[src="${source}"]`); | ||
|
||
if (element) { | ||
return Promise.resolve(); | ||
} | ||
|
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.
같은 src로 동시에 여러번 loadScript를 호출하면 어떻게 되나요?
load중일 때에는 아직 element가 만들어지기 전이라서, 같은 script를 여러 개를 만들 것 같아요. flag를 두어서 중복생성하지 않도록 막아주는 게 좋을 것 같습니다
📝작업 내용
스크립트를 로드하는 유틸 함수 구현
💬리뷰 참고사항
#️⃣연관된 이슈
close #575