-
-
Notifications
You must be signed in to change notification settings - Fork 669
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
add rollup plugin for building citation #3385
base: update-config-dependencies
Are you sure you want to change the base?
Conversation
|
packages/config/rollup.js
Outdated
@@ -144,6 +150,7 @@ export const makeRollupConfig = (iifeName) => | |||
outputOptions: { | |||
exports: "default", | |||
globals: { jspsych: "jsPsychModule" }, | |||
plugins: [cffToJsonPlugin()], |
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 haven't looked into the details yet, but have you tried playing with the plugin order? Briefly looking at the plugin code, I think the custom plugin should be at the end of the plugin chain.
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.
So this order works OK for the node.js builds when we don't have to bundle external dependencies. We thought it made sense to put it earlier because it's just doing a code injection and we thought having the stuff that comes after process that injection made sense.
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.
Just confirmed that moving the custom plugin to the end doesn't fix the issue.
@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 |
No description provided.