-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat: dfx new templates #3499
feat: dfx new templates #3499
Conversation
c885e83
to
60a541e
Compare
// redirect all requests from .raw.icp0.io to .icp0.io (this redirection is the default) | ||
"allow_raw_access": 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.
dfx 0.14.4 changed this default to true, and made it so we no longer specify the default for this field in these new-project .ic-assets.json5 files. Is it intended to change that behavior here?
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.
The others came from @krpeacock's templates, and then the empty one I was just making consistent with those. If we changed it away from this default for a reason I won't gainsay it.
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.
Yes. The new templates should use the new default (and not specify the default in .ic_assets.json5)
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.
These changes to defaults are the only reason I still have changes requested on this PR
I think it's a problem that these templates won't run correctly with |
Co-authored-by: Severin Siffert <[email protected]>
Is it? I don't picture a user generating a project with a newer version of dfx, then switching to an older one. |
Someone might share their code, generated from a new version, and then someone could have trouble running it if the version isn't pinned or indicated. Probably an edge case though. Still, having access to |
This comment was marked as resolved.
This comment was marked as resolved.
i know this is still WIP, but when attempting to create a new project, my frontend wasn't generated with the following output.
i can confirm that |
PS: i think this is amazing, it would be even cooler if templates with II would integrate with the frontend code as well. |
src/dfx/src/commands/new.rs
Outdated
|
||
#[derive(ValueEnum, Debug, Copy, Clone, PartialEq, Eq)] | ||
enum FrontendType { | ||
Svelte, |
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.
to be more accurate, can we use sveltekit instead of svelte here?
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.
@krpeacock do you have an opinion on this?
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.
@adamspofford-dfinity In the absence of an opinion from anyone with a better handle on the ecosystem, I think we should change this to SvelteKit (and the cli option to --type sveltekit
), since there is a distinction between the two.
support for e2e tests using |
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.
From my perspective, this is ready to go out (once all the checks pass)
I do plan to do this, but it'll be a follow-up |
src/dfx/src/commands/new.rs
Outdated
|
||
#[derive(ValueEnum, Debug, Copy, Clone, PartialEq, Eq)] | ||
enum FrontendType { | ||
Svelte, |
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.
@adamspofford-dfinity In the absence of an opinion from anyone with a better handle on the ecosystem, I think we should change this to SvelteKit (and the cli option to --type sveltekit
), since there is a distinction between the two.
assert_command dfx new e2e_no_frontend --no-frontend | ||
assert_file_exists e2e_no_frontend/src/e2e_no_frontend_frontend/assets/sample-asset.txt | ||
@test "frontend templates apply successfully" { | ||
for frontend in sveltekit vue react vanilla simple-assets none; do |
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 may be what is breaking
for frontend in sveltekit vue react vanilla simple-assets none; do | |
for frontend in svelte-kit vue react vanilla simple-assets none; do |
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.
No, I renamed it in the same commit.
Also closes #2771.