-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
Generate citation strings from citation.cff file as part of build process #3385
base: main
Are you sure you want to change the base?
Conversation
|
@jodeleeuw When you updated the dependencies, including esbuild from 0.15.14 to 0.23.1 in 59ce0b3, did you read the changelogs? There seem to be a lot of breaking changes in esbuild and we need to be confident that they are not breaking for @jspsych/config users (unless you are planning to release a config v4 for any other reason 🤔 ). I didn't get suspicious about this earlier because your commit message didn't make it clear to me that you were updating dependencies 🙈 |
"sucrase": "3.34.0", | ||
"tslib": "2.6.2", | ||
"typescript": "^5.2.2" | ||
}, | ||
"overrides": { |
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.
When I last checked a few months ago, NPM did only respect overrides defined in the root package.json
. Did that change in a recent NPM version? If so, we still can't rely on it until we require the new version...
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.
esbuild has a define
option to specify code replacements. I think we should be using that instead of a custom rollup plugin to keep it simple.
@jodeleeuw I double-checked and committed the rollup dependency updates in #3396 – they're all welcome and independent of the .json issue. Re that issue: Because we are using esbuild via the rollup plugin, the final bundling is performed by rollup. esbuild never sees |
@bjoluc just FYI, we ended up solving this by using |
Have you thought about declaring the constants globally in a Also, what about replacing a single |
For some reason I thought this kind of strategy wouldn't work, but can't reconstruct my reasoning now. We can explore it!
This was something that crossed my mind. I was thinking that the current boilerplate makes it easier for someone building with the JS template to modify because they won't have to know the syntax, but then again we could just have a different template for plugins that aren't going to go through the build process. So that does seem better. |
@cherriechang I'd like to get this merged soon. We need some docs still I think. @jadeddelta we could also include the CFF files in the v8 update for the contrib repo? |
… citation tool docs
… add-citation-module
No description provided.