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

Add new Function() to evaluate string for defaultValue from compodoc #17353

Closed

Conversation

amaino
Copy link

@amaino amaino commented Jan 27, 2022

Issue: #17004 #17001 #16959 #16865

What I did

Instead of returning the string for the default value from compodoc I ran the value through new Function(...) so the javascript in the string can be recreated to the original value from the component typescript file.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jan 27, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit 235e819. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@amaino amaino marked this pull request as ready for review January 27, 2022 14:08
@amaino amaino changed the title Add new Function() to evanluate string for defaultValue from compodoc Add new Function() to evaluate string for defaultValue from compodoc Jan 27, 2022
@amaino
Copy link
Author

amaino commented Jan 27, 2022

I have no idea what tests are failing does not seem related to what I have changed?

@yannbf
Copy link
Member

yannbf commented Feb 7, 2022

Hey @amaino thanks a lot for your contribution!

@storybookjs/angular I was wondering if any of you could jump and review this PR? Thank you!

@shilman I believe you have some ideas about this, care to share?

@yarrysmod
Copy link

any updates on this PR?

@amaino
Copy link
Author

amaino commented Mar 3, 2022

any updates on this PR?

@shilman wanted to do something different so waiting on that. My suggestion is that this PR is accepted in the meantime to get things working as I have to patch my storybook after installation to be able to use it, I guess I am not the only one were things are broken as it is now!

@david-genger
Copy link

Any update. this makes storybook docs unusable for angular. Please merge this.

@stirelli
Copy link

Pulling this up

@amaino
Copy link
Author

amaino commented Jun 6, 2022

Just upgraded to 6.5 still the same problem getting a string where I am expecting a function or an object. I have not heard a valid argument why we should have a broken system until we do things more correctly. Eval was used before, new Function() is a little better but perhaps not perfect but it works.

I am sorry @shilman but I do not understand what the argument is to have storybook broken without using patch-package after install. I need to redo my patch now though with 6.5 as the placement of files are different.

@Squixx
Copy link

Squixx commented Aug 16, 2022

Can we please include this fix?

@tmeasday
Copy link
Member

Sorry about the long delay here folks and thanks for the contribution @amaino!

This was actually a bug we fixed in the other frameworks and the fix is pretty simple: #19935 -- I'm going to close this in favour of that, if that's OK. Sorry for that.

@tmeasday tmeasday closed this Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants