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

Run bootstrap before linting on CI #1698

Closed
wants to merge 39 commits into from
Closed

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Aug 21, 2017

See https://storybooks.slack.com/archives/C4Q0S3MQR/p1503243475000043

I've added only those bootsrappers, lack of which produced issues for me. Maybe it's worth to add some more (or remove bootstrap:docs)

The issues I faced are:

  1. In, Stricter linting rules for imports #1676, the only reason we can't promote import/extensions to "error" level is that packages highlight.js and fuse.js are treated by this rule as local files unless they're actually installed
  2. In Use Flow instead of prop-types in lib/components #1696, I have to add dependencies to eslint-plugin-flowtype, eslint-plugin-flowtype-errors, and flow-bin to root package instead of lib/components so that they're installed with simple yarn install without bootstrapping

The main problem is that all those issues are discoverable only when it actually fails on CI (unless you run yarn bootstrapp -- --reset before each commit, after you made the changes)

@codecov
Copy link

codecov bot commented Aug 21, 2017

Codecov Report

Merging #1698 into release/3.3 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/3.3    #1698   +/-   ##
============================================
  Coverage        22.35%   22.35%           
============================================
  Files              325      325           
  Lines             6523     6523           
  Branches           813      808    -5     
============================================
  Hits              1458     1458           
- Misses            4459     4473   +14     
+ Partials           606      592   -14
Impacted Files Coverage Δ
app/react/src/server/config/babel.js 0% <0%> (-100%) ⬇️
addons/knobs/src/react/WrapStory.js 20% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/filters.js 47.36% <0%> (ø) ⬆️
addons/info/src/components/types/PrettyPropType.js 10.2% <0%> (ø) ⬆️
addons/knobs/src/base.js 2.5% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 23.25% <0%> (ø) ⬆️
addons/storyshots/src/require_context.js 45.58% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 22.41% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (ø) ⬆️
addons/info/src/components/PropTable.js 22.22% <0%> (ø) ⬆️
... and 19 more

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 41054ab...be743ef. Read the comment docs.

@Hypnosphi Hypnosphi added the maintenance User-facing maintenance tasks label Aug 21, 2017
@ndelangen
Copy link
Member

I'm not opposed to this at all, but please state the reasons clearly in the PR, Slack messages get lost over time, and the message you link to does not give a comprehensive picture of why this is exactly needed, thanks 👍

@Hypnosphi
Copy link
Member Author

I've added some clarifications

@Hypnosphi Hypnosphi changed the base branch from master to release/3.3 August 30, 2017 23:30
@@ -43,7 +43,7 @@ linkTo('Toggle') // Links to the first story in the 'Toggle' kind
With that, you can link an event in a component to any story in the Storybook.

- First parameter is the the story kind name (what you named with `storiesOf`).
-   Second (optional) parameter is the story name (what you named with `.add`). If the second parameter is omitted, the link will point to the first story in the given kind.
-   Second (optional) parameter is the story name (what you named with `.add`). If the second parameter is omitted, the link will point to the first story in the given kind.
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea =(

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Sep 6, 2017

#1810 should make this PR partially obsolete.

@ndelangen is there any solid reason why we bootstrap docs package separately from others?

@Hypnosphi
Copy link
Member Author

In fact, even with #1810 there's still a sense in running bootstrap: it removes a bunch of imports/no-unresolved warnings which are present because of lack of dist folders. This can allow to raise this rule to error level as well

@ndelangen
Copy link
Member

Well the reason for not bootstrapping docs along with core, is because often it's not needed.

I would like to be able to bootstrap sections individually.
So a developer can choose what to work on, and get some faster bootstrap.

BUt it's a nice to have, I do not oppose including docs into the default bootstrap, but it is unnecessary for 90% of the development that occurs currently. (And even if the code-change is in docs, often it can be worked on from github GUI or without running the docs app to begin with)

@Hypnosphi
Copy link
Member Author

Let‘s measure how much does it add up to clean bootstrap time

@Hypnosphi
Copy link
Member Author

Unfortunately, when I try to add docs to workspaces, the build command fails:

Error: Error: Cannot find module './addons/_template.jsx'.
    at render-page.js:31859:42
    at webpackContextResolve (render-page.js:31859:90)
    at webpackContext (render-page.js:31856:30)
    at render-page.js:25774:36
    at Array.forEach (<anonymous>:null:null)
    at module.exports (render-page.js:25744:27)
    at render-page.js:90:40
    at module.exports (render-page.js:31819:11)
    at Object.<anonymous> (render-page.js:89:2)
    at __webpack_require__ (render-page.js:30:30)

@Hypnosphi
Copy link
Member Author

Superseded by #1934

@Hypnosphi Hypnosphi closed this Oct 1, 2017
@Hypnosphi Hypnosphi deleted the ci-bootstrap-lint branch October 11, 2017 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants