[PLAT-8122] React19, Stencil4, & other framework tooling upgrades#763
[PLAT-8122] React19, Stencil4, & other framework tooling upgrades#763
Conversation
| export default config( | ||
| input('src/index.ts'), | ||
| typescript(), | ||
| output({ enableInlineDynamicImports: true }) |
There was a problem hiding this comment.
The common build-tool config was abandoned for this package. This was somewhat a decision I inherited from the initial draft PR ( #729 ). I have tried to understand if I could update build-tools instead, but haven't had too much success with that so far. Maybe for now there is a way to just incorporate this enableInlineDynamicImports bit into this custom config? But, also at a bit at a loss on how exactly to accomplish that, or how to test if it is done correctly. Looking for feedback on the later part from @amvertex since that was added recently by them.
There was a problem hiding this comment.
think I figured this out, not how to test it, but should be ready to test
add TODO: realted to build-tools
| public stencilBuffer: StencilBufferManager = new StencilBufferManager( | ||
| this.hostElement | ||
| ); | ||
| public stencilBuffer!: StencilBufferManager; |
There was a problem hiding this comment.
What was the reason behind this change?
There was a problem hiding this comment.
Related to something in the Stencil 4 upgrade. this could be undefined, so this.hostElement would error. Setting it inside of connectedCallback avoided that
| if (drawnFrame != null) { | ||
| if ( | ||
| drawnFrame != null && | ||
| frameRenderVersion === this.frameRenderVersion |
There was a problem hiding this comment.
What was the reason behind this change?
There was a problem hiding this comment.
IIRC this one was around trying to solve memory leak issue. Short story was in tests many viewers get started but teardown was not always cleaning them up or cleaning them up well. so would end up in SIGSEGV type errors where things were just running out of memory. This seemed like it improved the situation though it didn't ultimately solve it. Felt worth keeping, though in practice I don't imagine many users of the component would bump into cases where it would make much difference
| "name": "root", | ||
| "private": true, | ||
| "nextVersionBump": "patch", | ||
| "nextVersionBump": "major", |
There was a problem hiding this comment.
@jdm717 what do you think about releasing v1.0.0 on our next release?
There was a problem hiding this comment.
I didn't really mean to do this, sort of just wasn't connecting dots. I was thinking 'major' in the current semver context where minor == major. Well, pushed this to publish-testing branch before I put that together, so now we have a 1.0.0-testing build on NPM 😬
Eh, gotta go 1.0 at some point right?! 😅
There was a problem hiding this comment.
I think I'd be on board with shipping a v1.0.0 with our next release to signal the significant changes being introduced! Plus we're probably a bit overdue to release v1.0.0 - we've been on v0.x.x for just a few years 🙂
| @@ -0,0 +1 @@ | |||
| jest.spyOn(console, 'debug').mockImplementation(() => {}); | |||
There was a problem hiding this comment.
there was quite a bit of console noise in tests. The way I set this up it makes it pretty easy to mute and unmute that. just comment out this line and get all the debugging messages back.
eslint will be explicitly cjs until upgrade to v9
previous build structure
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
haven't existed for a while anyway
| before enabling this block. | ||
| --> | ||
|
|
||
| <!-- |
There was a problem hiding this comment.
@jdm717 sonar doesn't like these commented out bits, but IMO they are quite handy. Is there a way to tell Sonar to ignore these examples/**/index.html files ?
There was a problem hiding this comment.
It looks like I'm not able to change any of the rules there - I'll take a look at getting the correct permissions to do so next week, but in the meantime if you want to get these to stop showing up on the PR, you can always open the link and click the dropdown where it says Open and choose Accept in the cases where we're not going to take the action Sonar suggests, which should at least cause the Sonar check to pass. It does look like Sonar supports path exclusions though, so it should be possible to get the examples directory omitted later!
There was a problem hiding this comment.
I got around this. Ended up just putting this comment in the examples dir readme. Easy enough to grab it from there if doing local testing
There was a problem hiding this comment.
these move to .cjs files for now. Longer term goal would be to move to the new eslint v9 config files.
| @@ -1,7 +1,26 @@ | |||
| const jestConfig = require('@vertexvis/jest-config-vertexvis/jest.config'); | |||
| // import jestConfig from '@vertexvis/jest-config-vertexvis/jest.config'; | |||
| // TODO: return to using build-tool jest-config once supports ESM | |||
There was a problem hiding this comment.
Do we have a ticket for this?
There was a problem hiding this comment.
Not yet, but certainly a good idea. I have multiple followup type tasks I have noted locally. Think I just need to set aside some time for getting those onto a proper backlog
There was a problem hiding this comment.
Sounds good! I think it would be worthwhile to create the different tickets for future improvements.
| @@ -0,0 +1,9 @@ | |||
| const modulePath = new URL('../dist/components/vertex-document-viewer.js', import.meta.url); | |||
There was a problem hiding this comment.
Do you want to merge this file?
There was a problem hiding this comment.
Mixed thoughts. There is no inclusion in build or CI, and I don't think that would be necessary, but is it useful enough to stick around for occasional testing if a developer happens to be working on something that would have a chance to have a dependency on browser specific API?
doc-viewer-react and -vue
|



Summary
Modernization of dependencies and peer dependencies. Specifically to target official React 19 support, but along for the ride is TS 4 -> 5, Stencil 2 -> 4, Jest 27 -> 29, and few smaller bumps for things like ts-lib
Packaging / Module Format
This release updates the SDK’s packaging to prefer ESM and align package resolution with modern Node/bundler behavior. CommonJS remains supported, so most users should not need to change anything. The main cases to watch for are custom deep imports into dist/ or other package internals, and any hard-coded CDN/file-path references to emitted bundle filenames, which may need to be updated. Viewer packages were also adjusted to be safer to import in SSR/Node contexts.
Notes
This PR replaces #729 as that one needed significant merge conflict resolution and was getting out of hand
Test Plan
unit tests should pass
builds and link checks pass
will have -testing releases to test against nextjs starter with react 19. Vertexvis/vertex-nextjs-starter#224
nuxtjs-starter also tested with built packages: Vertexvis/vertex-nuxtjs-starter#8 to ensure vue wrappers remain functional and compatible
testing locally with yarn link
use
yarn examples:startin this repo to test new stencil componentsconnect app with testing builds or yarn linking should also look good
Release Notes
New major version.
Drop support for React < 17
Possible Regressions
Dependencies
nextjs and nuxtjs starter apps
Vertexvis/vertex-nextjs-starter#224
Vertexvis/vertex-nuxtjs-starter#6