fix: Compile TS to JS during packaging - #8000
Conversation
KoolADE85
left a comment
There was a problem hiding this comment.
Left a couple comments on the testing, but the solution itself is working well on my end.
| const { createRequire } = require('node:module'); | ||
| const path = require('node:path'); |
There was a problem hiding this comment.
Claude took me down this path as well while I was testing this out. To me, it feels a bit "one step removed" from a real-world validation. Could we instead write a more idiomatic import Plotly from "plotly.js"; and then assert the same things?
There was a problem hiding this comment.
This was all an attempt to run without npm install, but it's too complicated. I'll remove it in favor of GHA.
There was a problem hiding this comment.
Is there a particular reason you write this as a util here?
To me, the file reads like a github action written in javascript. And it's doing some bizarre setup along the way that make the assertions seem a bit artificial.
Meanwhile, what we actually care about:
- Install the lib
- Import the lib
- Assert it worked
And that seems like an ideal case for a GH action or even just a unit test.
There was a problem hiding this comment.
You're right. I liked having the test locally, but it does fit better in GHA. I'll remove this file.
camdecoster
left a comment
There was a problem hiding this comment.
Per your suggestion, I moved the test into GHA and removed the local files.
| const { createRequire } = require('node:module'); | ||
| const path = require('node:path'); |
There was a problem hiding this comment.
This was all an attempt to run without npm install, but it's too complicated. I'll remove it in favor of GHA.
There was a problem hiding this comment.
You're right. I liked having the test locally, but it does fit better in GHA. I'll remove this file.
Description
Compile TS down to JS during packaging (before publishing) to fix issue with Node resolution.
Closes #7995.
Also fixes TS issues discovered during investigation:
Datatype referencesChanges
.tsfiles from packagingTesting
npm ciNotes
node_modulesmain). Node therefore follows that path to resolve the package and it throws an error when it encounters a TS file.Datatype inindex.d.tswas being imported incorrectly