Skip to content
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

Fix: --no-codegen option is now recognized in polywrap build #1618

Merged
merged 4 commits into from
Mar 13, 2023

Conversation

krisbitney
Copy link
Contributor

fix for issue where noCodegen option name is automatically changed to codegen by Commander package in build command

…to `codegen` by Commander package in build command
pileks
pileks previously approved these changes Mar 10, 2023
Copy link
Contributor

@pileks pileks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged ASAP, as it also fixes the IPFS hash issue! :D

Full disclosure: I was the one that changed this codegen stuff, and I left it as such since, with Commander, specifying a --codegen flag also adds the option of --no-codegen as an alternative flag. I did exclude the noCodegen option for the CLI-JS, as I didn't think of it at the time. Good catch @krisbitney!

cbrzn
cbrzn previously approved these changes Mar 10, 2023
pileks
pileks previously approved these changes Mar 10, 2023
Copy link
Contributor

@pileks pileks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo @cbrzn!

@cbrzn cbrzn dismissed stale reviews from pileks and themself via fcf0dfb March 13, 2023 10:26
@cbrzn
Copy link
Contributor

cbrzn commented Mar 13, 2023

i've (in theory) fixed all tests except for create app typescript command.

this last test, is failing in this line https://github.com/polywrap/toolchain/blob/kris/fix-codegen-option-recognition/packages/cli/src/lib/project/templates/generateProjectTemplate.ts#L142-L149 and the reason is that we've done a change in .pre-* in the app templates (now we only have typescript). and it is pulling the latest in npm (which points to 0.9.6); hence, it is trying to copy a folder that doesn't exist.

I've seen this problem before, but I do not remember what fixed it... Also, it's worth mentioning that this error is totally unrelated to this PR, and it was introduced two weeks ago in this PR #1529 (origin-dev has been broken since then).

i think a hotfix might be to hard code the current version of @polywrap/templates (we can just do @polywrap/templates@0.10.0-pre.11 and add a todo to remove it once 0.10 is relased), but I feel this is no bueno tho - open to any suggestions 😄

@krisbitney @pileks

@dOrgJelli
Copy link
Contributor

Thank you @krisbitney! Let's fix the create ... CLI test in another PR, no need to hold this up longer.

@dOrgJelli dOrgJelli merged commit 4e190f8 into origin-dev Mar 13, 2023
@dOrgJelli dOrgJelli deleted the kris/fix-codegen-option-recognition branch April 10, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants