Conversation
jjti
left a comment
There was a problem hiding this comment.
mostly LGTM just that one question, demo rewrite looks legit thank you for doing that
Co-authored-by: Joshua Timmons <joshua.timmons1@gmail.com>
jjti
left a comment
There was a problem hiding this comment.
one more react version question, we've got a dependency on react-resize-detector but it requires react 18||19, is there a lower version of react-resize-detector that lets us keep support for lower versions of react? https://github.com/maslianok/react-resize-detector/blob/4671b05c024fb8c75fbb1a8c68d749c3845adad4/package.json#L45
Hey @jjti , just to let you know that I've seen this. Some other priorities came up, but I'll get to it soon. |
Not really, we'd need to go down to react-resize-detector v9 to be able to support React 16, but that would not allow us to support React 19: A similar thing happens with React 17. We would need to go down to react-resize-detector v10, but that doesn't support React 19 either. What I did was remove React 16 and 17 from Seqviz dependencies. Let me know what you think. |
|
@jjti any other thoughts? |
This is, unfortunately, a breaking change. To do that and maintain the spirit of semver we'd need to bump seqviz's major version. But then users of seqviz are unlikely to also go and upgrade their seqviz major version and users might not get upgrades to seqviz (we'd also be abandoning anyone on react 16 or 17). There is a related discussion here: maslianok/react-resize-detector#253 While yes, we definitely want to add React 18/19 support, I think the most correct thing here is to try to either:
|
@jjti I see your points and make sense! I forked Let me know what you think |
| ], | ||
| "dependencies": { | ||
| "react-resize-detector": "^7.1.2", | ||
| "react-resize-detector": "github:Lattice-Automation/react-resize-detector#adding-older-peer-dependencies", |
There was a problem hiding this comment.
can we publish our fork to npm and fix this up a bit? Every user of seqviz will get this as a dependency
There was a problem hiding this comment.
Yeap, I can do that. I wasn't sure what was the best practice for forked repos. Seems strange to publish a package from a forked repo.
What do you think should be the name? @lattice-automation/react-resize-detector seems like the best option but I don't think we have a lattice-automation org in npm (or at least Seqviz doesn't seem to be part of an org).
I could also do something like react-resize-detector-fork and upload it under my own account but by your previous comment, we want to do it as cleanly as possible, so not sure.
There was a problem hiding this comment.
@lattice-automation/react-resize-detector seems perfect/ideal to me. Yeah starting over seqviz would be in an org-prefixed package like that but the ship sailed
There was a problem hiding this comment.
Yeah I guess at some point we could deprecate this package and move it to an the org. I've seen that in other packages.
I'll talk to @leshane to set up the Lattice org. We'll probably need it for other packages soon too.
There was a problem hiding this comment.
@jjti Kevin set up a Lattice Org and I've added the react-resize-dependency there. Let me know what you think.
|
@jjti any more thoughts? |
|
LGTM thanks for keeping support for older React versions |
The main motivation for this was to fix #287. Because the demo app used react 18 and semantic UI, and semantic UI is not supported in react 19, I ended up migrating the app to use material UI.
The side effect of all this upgrade is that we removed all vulnerabilities:
Previously:
Now:
Here's also a comparison of both app (material ui and semantic UI):
Screen.Recording.2025-10-15.at.16.03.39.mov
I also had to update one test. The test was for a deprecated feature (file) so I did not pay too much attention to it. In any case, the test seemed to be looking at "the first part of the sequence being rendered" which seems arbitrary.
Note:
I had to add a few exceptions to ESLint rules. This is to mimc the old behavior. Not sure why those rules were not throwing errors before. I suppose it was because old ESLint didn't enforce them.
I also had to remove one rule
@typescript-eslint/padding-line-between-statementsthis is because this rule is no longer in that package, but in another (@stylistic/eslint-plugin) that can't be installed because has dependency conflicts witheslint-plugin-typescript-sort-keys