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

env: update prosemirror versions (fix #2631) #2648

Merged
merged 7 commits into from
Aug 5, 2022

Conversation

jajugoguma
Copy link
Contributor

Please check if the PR fulfills these requirements

  • It's the right issue type on the title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has a description of the breaking change

Description

  • Fixed that it can be installed and used normally in response to the ProseMirror update and its changes.
    • The minor update of ProseMirror had some breaking changes, so to prevent this, I had to fix the version before.
    • However, this caused a problem that did not install or run normally.
    • So I updated it in response to the breaking changes of ProseMirror.

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

@jajugoguma jajugoguma requested a review from jwlee1108 August 5, 2022 05:03
Copy link
Contributor

@jwlee1108 jwlee1108 left a comment

Choose a reason for hiding this comment

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

꼼꼼하게 비교하는거 힘들었을텐데 고생하셨어요.

"prosemirror-state": "~1.3.4",
"prosemirror-view": "~1.18.7",
"prosemirror-transform": "~1.3.0"
"prosemirror-commands": "^1.1.9",
Copy link
Contributor

Choose a reason for hiding this comment

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

이 형태로 가도 괜찮나요? 앞으로 breaking change가 발생할 마이너 변경에 대응을 계속 해줘야할까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이와 관련해서 의견을 조금 여쭙고자합니다.
이전 시도에서와 같이, 버전을 고정하면 결국 특정 환경에서 정상 동작하지 않는 문제가 발생합니다.
그러나 버전을 고정하지 않으면 타입과 관련된 여러 문제가 발생하지만, 사용자 측에서 동작 자체는 대부분 잘 동작하는 것으로 보입니다.

그래서 결국에는 마이너 업데이트에 breaking change가 있을 경우 대응을 해주어야 합니다만, 사용자가 큰 불편을 겪지 않는 것은 현재형태로 가는 것이 제가 보기엔 나아보이는데, 팀장님 생각은 어떠신가요?

Copy link
Contributor

Choose a reason for hiding this comment

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

음 그럼 ^으로 바꾸죠. 대신 그럼 ci 스크립트를 npm ci에서 npm install로 바꿉시다. 그래야 미리미리 추적이 가능할 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵! github actions 수정하도록 하겠습니다.

<span><a href="www.google.com">@test1</a></span>
</span>
<span class="tui-widget">
<span>
Copy link
Contributor

Choose a reason for hiding this comment

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

구조가 좀 더 심플하게 바뀌었나보네요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

구조가 심플하게 바뀐건 아니고, 상위 요소가 paragraph이고 silbling으로 break 요소로 가지고 있습니다. 그런데 ProseMirror 업데이트로 인해 break 요소에 클래스네임이 추가되었습니다. 그러나 확인하려는 요소는 결국 paragraph 내부의 요소들이기 때문에 실제 테스트하고자 하는 요소들만 남겼습니다.

@@ -22,6 +22,18 @@ export function removeDataAttr(html: string) {
return html.replace(/\sdata-nodeid="\d{1,}"/g, '').trim();
}

export function removeProseMirrorHackNodes(html: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

�일부러 함수를 새로 만든건가요? 이러면 코드 내 함수랑 두 벌로 관리해야 하지 않아요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 이 함수는 제가 만들고 사용하지 않던거라 삭제하겠습니다.

@@ -33,6 +33,7 @@ function createTableFragment(tableHead: Node, tableBody: Node) {
return Fragment.from(fragment);
}

// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

ts-ignore는 이유가 별도로 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Selection을 구현한 CellSectiontoJSON 메서드를 구현해야하는데 작성하지 않아도 될 것 같아 ts-ignore 처리를 했습니다.

그런데 지금 생각해보니 나중을 위해선 그냥 toJSON을 구현하는 것이 나을 것 같네요.
ignore 처리 삭제하고 toJSON을 구현하는 것으로 변경하겠습니다.

@@ -33,6 +33,7 @@ function createTableFragment(tableHead: Node, tableBody: Node) {
return Fragment.from(fragment);
}

// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 ts-ignore 있는데 확인 부탁드려요.

test: fix failing tests
chore: implement method that was not
chore: remove unused utility function
@jajugoguma jajugoguma merged commit 65c4983 into master Aug 5, 2022
@jajugoguma jajugoguma deleted the env/update-prosemirror branch August 5, 2022 06:40
ahamelers pushed a commit to ahamelers/tui.editor that referenced this pull request Aug 21, 2023
* env: update prosemirror versions

* chore: fix types according to prosemirror updates

* chore: remove something added to DOM by prosemirror

* test: fix tests following prosemirror updates

* chore: fix type errors

* chore: apply code reviews

test: fix failing tests
chore: implement method that was not
chore: remove unused utility function

* chore: change npm ci to npm install in ci test workflows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants