-
Notifications
You must be signed in to change notification settings - Fork 3
feat: new vite-friendly /utils exports
#595
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
Conversation
- introduces typescript config - converts a bunch of files to typescript - creates a new export object that exposes utils directly - adds `ts-loader` so webpack can handle typescript - moves to vitest for test runner
this gets the legacy bundles working (i think)
This reverts commit 3226d1a.
- use native node.js glob - remove prettier@2 - add in @csstools/css-parser-algorithms so tests can work in CI?
/utils exports
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.
this is a barrel file that exposes the util functions so they can be accessed via the new @readme/syntax-highlighter/utils export
| optimization: { | ||
| minimize: true, | ||
| minimizer: [new TerserPlugin()], | ||
| minimize: false, |
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.
i don't think we should be optimizing in this repo for a few reasons:
- in my local testing, our legacy webpack build in our main repo can't handle these changes (which are so small so i can't really fathom why)
- harder to troubleshoot (i generally am opposed to minification for npm libraries for this reason)
- this package is being fed into bundlers in our downstream repos and i'd rather let those take care of the minification
| { | ||
| test: /\.tsx?$/, | ||
| use: { | ||
| loader: 'ts-loader', |
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.
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.
ts-loader? i dont ts-knower
| { | ||
| test: /\.tsx?$/, | ||
| use: { | ||
| loader: 'ts-loader', |
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.
ts-loader? i dont ts-knower
| import canonical from './utils/canonical'; | ||
| import { modes } from './utils/modes'; | ||
| import uppercase from './utils/uppercase'; | ||
| import canonical from './utils/canonical.ts'; |
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.
maybe you answered this somewhere, but why did you have to add the .ts to these imports?
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.
good q! i added these because eslint was yelling at me 🤷🏽 pretty sure webpack can handle these whether or not they have file extensions


🧰 Changes
was seeing this error when attempting to load a vite server module in our main repo:
this is happening because vite is a bit stricter about how it resolves modules compared to webpack. this library has a mix of default/non-default exports and ill-defined exports that are throwing off vite's module resolver.
in order to address this, i've added a new
utilsexport (i.e.,import { uppercase } from '@readme/syntax-highlighter/utils') that exposes fully-typed versions of these util functions. all of these util functions are vanilla JS and zero deps so if we switch to those imports, i bet we'll see better treeshaking.in addition to the above, i went through and did some routine cleanup in this repo since i've barely touched it prior to now:
ts-loaderso webpack can properly handle typescript (my god i hate webpack)knipinto the mix and cleared out a bunch of deps as a result🧬 QA & Testing
again, there should be no functional changes, just introducing a bit more API surface area so bundlers like vite can play nicely with this library.
not sure how to test this other than ensuring that tests pass? would love some pointers on how to QA this! to be safe, i'd recommend we release this as a beta and test it out in our main repo before publishing it to the main
latestchannel.Footnotes
there are a handful of files that i didn't get around to converting to TS and i think it's fine for now. ↩
even though i think the main changes should be backwards compatible, i bet it'll break certain bundler setups so i think we should do a major version bump. hence i bumped the minimum node.js requirements here. in bumping our minimum node version, i was also able to remove the
globdependency and use nativefs.globSync😌 ↩