-
Notifications
You must be signed in to change notification settings - Fork 19
build_tool: update cjs to v0.19.0 and bump package version #378
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
build_tool: update cjs to v0.19.0 and bump package version #378
Conversation
1c3a246
to
9e1d0e0
Compare
Signed-off-by: Karthik Ganeshram <[email protected]>
9e1d0e0
to
da0465f
Compare
I think the best thing for now would be to make it a hard error instead of a warning: we shouldn't just silently build something with very different properties from what the user expected.
Given that we've never documented this, I think I agree that it's okay not to treat this as a breaking change, yeah. Over in CJS, we decided that it needs to be a breaking change, but there it was fully documented, so the situation was different. |
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.
LGTM with comment addressed.
packages/build-tools/src/index.ts
Outdated
console.warn( | ||
'Warning: AOT compilation is temporarily disabled due to issues with componentize-js. Proceeding without AOT. It may be removed in future releases.', | ||
); | ||
CliArgs.aot = 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.
As mentioned, I think we should make this a hard error, not a warning.
console.warn( | |
'Warning: AOT compilation is temporarily disabled due to issues with componentize-js. Proceeding without AOT. It may be removed in future releases.', | |
); | |
CliArgs.aot = false; | |
throw new Error( | |
'AOT compilation is currently unavailable. Remove the `aot` option to proceed.', | |
); |
Additionally, I think we should add a hidden: true
field to the yargs options for aot
over in cli.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.
Between this option and potentially just removing the option. Do you have a preference?
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 think this option would be better: we don't know if someone looked at the --help
output or the source and is using this in the wild after all. In that case, getting an explanation is definitely better than the option just disappearing.
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.
Done will update.
Signed-off-by: Karthik Ganeshram <[email protected]>
Bumps ComponentizeJS which brings in a newer build of StarlingMonkey but we loose
aot
. I have currently not removed the option but just print a warning and continue with building without it. If there are strong preferences, I am happy to delete it.It will be a breaking change, strictly speaking, but I do not think
aot
has been advertised in the Spin docs, and I'm also not sure if it is widely used.