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

v3 Theme Generator legacy theme import doesn't support comments #3084

Open
endigo9740 opened this issue Jan 7, 2025 · 2 comments
Open

v3 Theme Generator legacy theme import doesn't support comments #3084

endigo9740 opened this issue Jan 7, 2025 · 2 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@endigo9740
Copy link
Contributor

Current Behavior

If the v2 theme source contains comments, you'll see the following error:

Screenshot 2025-01-07 at 2 17 37 PM

Expected Behavior

By stripping out all comments, the theme will import as expected. Ideally we should strip the comments automatically for users as the raw v2 generator export includes these comments.

Steps To Reproduce

  1. Generate a new v2 theme
  2. Export to a theme.ts file
  3. Try import this via the v3 generator import feature
  4. The theme will fail with the above error
  5. Strip out any and all comments ad import again
  6. The theme will import as expected

Link to Reproduction / Stackblitz

No response

More Information

Reported by user @Fefefo on Discord.

@endigo9740 endigo9740 added the bug Something isn't working label Jan 7, 2025
@endigo9740 endigo9740 added this to the v3.0 (Next) milestone Jan 7, 2025
@phamduylong
Copy link
Contributor

phamduylong commented Jan 7, 2025

My sneaky suspicion is that those comments at end of lines in the v2 theme source are causing this trouble. The comments that have their own lines shall be fine.

// This line is fine
// primary | #c93655
// This line shall not pass 
"--color-primary-50": "247 225 230", // #f7e1e6

The parsing done in import-file.ts doesn't account for comments at the end of a line.

EDIT: I'm pretty sure that's the cause, it's not a sneaky suspicion.

@endigo9740
Copy link
Contributor Author

endigo9740 commented Jan 8, 2025

@phamduylong likely so, but don't worry about this. Hugo has volunteered to upgrade the portion that parses the file contents using the knowledge he's accrued from writing the migration CLI. It's a similar concept reading/parsing file contents, rather than how I'm doing it (reading it as text content).

@Hugos68 I'll go ahead and assign this one to you to represent those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants