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

storysource: Add default parser option. Support prettier v1.13.0 #3660

Merged
merged 10 commits into from Jun 3, 2018

Conversation

2 participants
@isoppp
Copy link
Contributor

isoppp commented May 28, 2018

Issue: #3657

What I did

Add default parser option for prettier v1.13.0 in storysource

How to test

Is this testable with Jest or Chromatic screenshots?

Probably can't.

Does this need a new example in the kitchen sink apps?

No.

Does this need an update to the documentation?

No.

Note

I tried add parser: 'babylon' in default option. And it works.
But I do not know if this modification alone is enough.

I try to make a PR of this comment on the ticket

@Hypnosphi

This comment has been minimized.

Copy link
Member

Hypnosphi commented May 29, 2018

Looks like it doesn't recognize typescript now:

[12:02:47][Step 2/2]   ● inject-decorator › positive - ts › encountered a declaration exception
[12:02:47][Step 2/2] 
[12:02:47][Step 2/2]     SyntaxError: Unexpected token (10:11)
[12:02:47][Step 2/2]        8 | })
[12:02:47][Step 2/2]        9 | class WithStoreComponent {
[12:02:47][Step 2/2]     > 10 |   private store: Store<any>;
[12:02:47][Step 2/2]          |           ^
[12:02:47][Step 2/2]       11 | 
[12:02:47][Step 2/2]       12 |   constructor(store: Store<any>) {
[12:02:47][Step 2/2]       13 |     this.store = store;
[12:02:47][Step 2/2] 
[12:02:47][Step 2/2]       65 | }
[12:02:47][Step 2/2]       66 | 
[12:02:47][Step 2/2]     > 67 | export function generateStorySource({ source, ...options }) {
[12:02:47][Step 2/2]       68 |   let storySource = source;
[12:02:47][Step 2/2]       69 | 
[12:02:47][Step 2/2]       70 |   storySource = generateSourceWithoutUglyComments(storySource, options);

CC @igor-dv

@@ -5,6 +5,7 @@ const defaultOptions = {
bracketSpacing: true,
trailingComma: 'es5',
singleQuote: true,
parser: 'babylon',

This comment has been minimized.

@Hypnosphi

Hypnosphi May 29, 2018

Member

We actually add parser option on call site

So, when parser === 'javascript' we should add parser: 'babylon' instead of skipping this part

But I think a more elegant solution would be to pass filepath option to prettier so that it could infer the correct parser from extension. You can get file path in transform function as this.resourcePath, add it to options object, and then pass to prettier instead of parser

This comment has been minimized.

@Hypnosphi

Hypnosphi May 29, 2018

Member

Both solutions would fix #3660 (comment)

This comment has been minimized.

@isoppp

isoppp May 30, 2018

Author Contributor

Thank you for information! I will try it.

This comment has been minimized.

@isoppp

isoppp May 30, 2018

Author Contributor

I fixed this.
Pass this.resourcePath to prettier config.

Thank you for telling me the information.
It was very helpful in fixing it.

isoppp added some commits May 30, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 30, 2018

Codecov Report

Merging #3660 into master will decrease coverage by 0.01%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3660      +/-   ##
==========================================
- Coverage   41.55%   41.54%   -0.02%     
==========================================
  Files         458      458              
  Lines        5058     5062       +4     
  Branches      849      851       +2     
==========================================
+ Hits         2102     2103       +1     
- Misses       2451     2453       +2     
- Partials      505      506       +1
Impacted Files Coverage Δ
addons/storysource/src/loader/index.js 0% <0%> (ø) ⬆️
addons/storysource/src/loader/parsers/parser-js.js 100% <100%> (ø) ⬆️
addons/storysource/src/loader/inject-decorator.js 100% <100%> (ø) ⬆️
addons/storysource/src/loader/parsers/parser-ts.js 100% <100%> (ø) ⬆️
addons/storysource/src/loader/generate-helpers.js 90.62% <66.66%> (-9.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd867d2...6ac4983. Read the comment docs.

isoppp added some commits May 30, 2018

@isoppp

This comment has been minimized.

Copy link
Contributor Author

isoppp commented May 30, 2018

I fixed one point with additional.
Since Prettie's parser was similarly changed in version 1.13.0,
( see: prettier/prettier@52a2a46 )
When the test was executed, the function called in parser-js.js etc outputted not function error.

And I did not do the test locally. I'm sorry.
In this time, we confirmed that errors are not generated by executing the test locally.

However, it seems that errors occur in places which are not touched by CircleCI.
How can i solve this?
Tell me if you have a good idea.

@isoppp isoppp changed the title storysource: Add default parser option. Support prettier v0.13.0 storysource: Add default parser option. Support prettier v1.13.0 May 30, 2018

@@ -25,7 +25,7 @@
"babel-runtime": "^6.26.0",
"estraverse": "^4.2.0",
"loader-utils": "^1.1.0",
"prettier": "~1.12.1",
"prettier": "^1.12.1",

This comment has been minimized.

@Hypnosphi

Hypnosphi May 30, 2018

Member

Please update it to latest version: looks like we don't support 1.12 after change in parser imports

This comment has been minimized.

@isoppp

isoppp May 31, 2018

Author Contributor

That is true
I changed to ^1.13.3

if (parser) {
config = {
...prettierConfig,
parser: parser === 'javascript' ? 'babylon' : parser,

This comment has been minimized.

@Hypnosphi

Hypnosphi May 30, 2018

Member

Not needed anymore, filepath is enough

This comment has been minimized.

@isoppp

isoppp May 31, 2018

Author Contributor

I thought the same.
But in this test , if the extension is txt and the contents are something else, prettier does not support to txt file, so it gets an error.

error:

No parser could be inferred for file: (LOCAL_PATH)/addons/storysource/src/loader/__mocks__/inject-decorator.stories.txt

Should i change the test?
I think that probably this is a rare case, so I think that do not have to deal with it unless need it.

This comment has been minimized.

@Hypnosphi

Hypnosphi May 31, 2018

Member

After giving it a second thought, we probably should always pass parser to prettier when user specifies it explicitly, which is exactly what we do now. So let's keep it as is

This comment has been minimized.

@Hypnosphi

Hypnosphi May 31, 2018

Member

But we probably should put else if (filepath) inside if (!config.parser), to cover cases when parser is not passed

This comment has been minimized.

@Hypnosphi

Hypnosphi May 31, 2018

Member

Or maybe we should just pass babylon when parser isn't set explicitly, because this is what we use by default ourselves. In this case, filepath will be never needed

This comment has been minimized.

@isoppp

isoppp May 31, 2018

Author Contributor

Thank you for review. I tried fixing it.

And, i read Prettier code a little.
Prettier gives priority to parser over filepath.
If both are empty we gave babylon as Prettier will output errors in the future.

if (parser) {
config = {
...prettierConfig,
parser: parser === 'javascript' ? 'babylon' : parser,

This comment has been minimized.

@Hypnosphi

Hypnosphi May 31, 2018

Member

let's put ...prettierConfig after this line, so that it wouldn't override explicitly passed prettierConfig.parser

This is why there was !config.parser check before

This comment has been minimized.

@isoppp

isoppp May 31, 2018

Author Contributor

I made a slight change to the beginning of this function.
When prettierConfig.parser is explicitly set return will be done at this point

if (prettierConfig.parser) return prettier.format(source, prettierConfig);

After this processing, shoud not set prettierConfig.parser.
So I do not think it is necessary to change it

This comment has been minimized.

@isoppp

isoppp May 31, 2018

Author Contributor

But, if this code is not good, I think it can try changes.

This comment has been minimized.

@Hypnosphi

Hypnosphi Jun 1, 2018

Member

I don't feel really good about this double prettier.format call in different logical branches. Let's maybe revert it to how it was, with if (!prettier config.parser) check?

This comment has been minimized.

@isoppp

isoppp Jun 2, 2018

Author Contributor

OK, I understood.
Many thanks for having you review it over and over. And, sorry...

I reverted before commit.
After, modified to make parser or filepath always specified
If the parser and file path are empty, set the parser babylon

@Hypnosphi Hypnosphi merged commit 4066e6f into storybooks:master Jun 3, 2018

32 of 33 checks passed

ci/circleci: lint Your tests failed on CircleCI
Details
Angular (Storybook) TeamCity build finished
Details
Better Code Hub ✅ Better Code Hub approves this code
Details
Build (Storybook) TeamCity build finished
Details
CLI test (Storybook) TeamCity build finished
Details
CLI test, latest CRA (Storybook) TeamCity build finished
Details
CRA (Storybook) TeamCity build finished
Details
Chromatic (Storybook) TeamCity build finished
Details
CodeFactor 2 issues fixed. 2 issues found.
Details
Danger (Storybook) TeamCity build finished
Details
Docs (Storybook) TeamCity build finished
Details
Examples (Storybook) TeamCity build finished
Details
HTML (Storybook) TeamCity build finished
Details
Lint (Storybook) TeamCity build finished
Details
Marko (Storybook) TeamCity build finished
Details
Mithril (Storybook) TeamCity build finished
Details
Node Security No known vulnerabilities found
Details
Polymer (Storybook) TeamCity build finished
Details
React Native (Storybook) TeamCity build finished
Details
Smoke tests (Storybook) TeamCity build finished
Details
Test (Storybook) TeamCity build finished
Details
Vue (Storybook) TeamCity build finished
Details
ci/chromatic 140 specs unchanged.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: cli Your tests passed on CircleCI!
Details
ci/circleci: cli-latest-cra Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: example-kitchen-sinks Your tests passed on CircleCI!
Details
ci/circleci: react-native Your tests passed on CircleCI!
Details
ci/circleci: smoke-tests Your tests passed on CircleCI!
Details
ci/circleci: unit-test Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details
security/snyk No dependency changes
Details

@Hypnosphi Hypnosphi removed the patch label Jun 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment