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.

Sign up for GitHub

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

Jump to bottom

Upgrade react-syntax-highlighter to the latest version #8266

Merged
merged 4 commits into from
Oct 7, 2019
Merged

Upgrade react-syntax-highlighter to the latest version #8266

merged 4 commits into from
Oct 7, 2019

Conversation

stof
Copy link
Contributor

@stof stof commented Oct 2, 2019 •
edited by shilman
Loading

Issue: #8208 #8038

What I did

react-syntax-highlighter is updated to the latest version, which is using an uptodate version of regenerator-runtime rather than the old 0.11 one.

How to test

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

@vercel
Copy link

vercel bot commented Oct 2, 2019 •
edited
Loading

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-stof-upgradesyntaxhighlighter.storybook.now.sh

@stof
Copy link
Contributor Author

stof commented Oct 2, 2019

needs DefinitelyTyped/DefinitelyTyped#38776 to be merged and released to fix typescript.

@stof stof force-pushed the upgrade_syntax_highlighter branch from de02c7f to 0979cc2 Compare October 2, 2019 09:21
stof
stof commented Oct 2, 2019
View reviewed changes
addons/storysource/src/StoryPanel.js
@@ -4,7 +4,7 @@ import { styled } from '@storybook/theming';
import { Link } from '@storybook/router';
import { SyntaxHighlighter } from '@storybook/components';

import createElement from 'react-syntax-highlighter/create-element';
import createElement from 'react-syntax-highlighter/dist/esm/create-element';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be checked again based on the comment in react-syntax-highlighter/react-syntax-highlighter#141 (comment)

@ndelangen
Copy link
Member

Very much appreciated if you could get everything fixed, and upgraded 👏👏👏

@stof
Copy link
Contributor Author

stof commented Oct 2, 2019

@ndelangen as I commented, I need to wait for the merge of the bugfix for types, as that's the main blocker (typescript thinks the sub-module does not exist because types tell it so). This is what's breaking builds for Travis, CircleCI as well as deployments.

@stof
Copy link
Contributor Author

stof commented Oct 2, 2019

@ndelangen any hint at what the DangerJS failed workflow means for my PR ?

@stof
Copy link
Contributor Author

stof commented Oct 2, 2019 •
edited
Loading

Looks like it is danger/danger-js#918 That's not good for a required check

@ndelangen
Copy link
Member

@stof We use PR labels to generate changelogs. So it's important that before merging the correct labels are applied. this is what Danger warns us for. It blocks Merging until labels are applied correctly.

@ndelangen ndelangen added this to the 5.3.0 milestone Oct 2, 2019
@shilman
Copy link
Member

shilman commented Oct 2, 2019

$ node ../../scripts/prepare.js
src/syntaxhighlighter/syntaxhighlighter.tsx(6,17): error TS7016: Could not find a declaration file for module 'react-syntax-highlighter/dist/esm/languages/prism/jsx'. '/tmp/storybook/node_modules/react-syntax-highlighter/dist/esm/languages/prism/jsx.js' implicitly has an 'any' type.
If the 'react-syntax-highlighter' package actually exposes this module, consider sending a pull request to amend 'https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-syntax-highlighter`
src/syntaxhighlighter/syntaxhighlighter.tsx(7,18): error TS7016: Could not find a declaration file for module 'react-syntax-highlighter/dist/esm/languages/prism/bash'. '/tmp/storybook/node_modules/react-syntax-highlighter/dist/esm/languages/prism/bash.js' implicitly has an 'any' type.
If the 'react-syntax-highlighter' package actually exposes this module, consider sending a pull request to amend 'https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-syntax-highlighter`
src/syntaxhighlighter/syntaxhighlighter.tsx(8,17): error TS7016: Could not find a declaration file for module 'react-syntax-highlighter/dist/esm/languages/prism/css'. '/tmp/storybook/node_modules/react-syntax-highlighter/dist/esm/languages/prism/css.js' implicitly has an 'any' type.
If the 'react-syntax-highlighter' package actually exposes this module, consider sending a pull request to amend 'https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-syntax-highlighter`
src/syntaxhighlighter/syntaxhighlighter.tsx(9,18): error TS7016: Could not find a declaration file for module 'react-syntax-highlighter/dist/esm/languages/prism/markup'. '/tmp/storybook/node_modules/react-syntax-highlighter/dist/esm/languages/prism/markup.js' implicitly has an 'any' type.
If the 'react-syntax-highlighter' package actually exposes this module, consider sending a pull request to amend 'https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-syntax-highlighter`

@stof stof force-pushed the upgrade_syntax_highlighter branch from 0979cc2 to 2748409 Compare October 4, 2019 09:53
@stof
Copy link
Contributor Author

stof commented Oct 4, 2019

@ndelangen couldn't it report that in the github actions logs instead, which would work for fork PRs too ?

@stof
Copy link
Contributor Author

stof commented Oct 4, 2019

@shilman I know about that, and I even commented about that in the PR previously.

I just pushed an update to that PR now that fixes for the types have been merged an released.

@stof stof force-pushed the upgrade_syntax_highlighter branch from 2748409 to d99d96c Compare October 4, 2019 10:54
@vercel vercel bot temporarily deployed to staging October 4, 2019 10:54 Inactive
@ndelangen ndelangen self-assigned this Oct 4, 2019
@stof
Copy link
Contributor Author

stof commented Oct 4, 2019

It looks like the testsuite does not like ES6 imports in the dependencies. I could switch to importing the CommonJS files (with another update to the types, as these are not documented there), but that would be a bad news for tree-shaking in webpack builds.

@ndelangen any idea how to make Jest work here ?

@ndelangen
Copy link
Member

Visual changes look very good!

@ndelangen
Copy link
Member

any idea how to make Jest work here ?

🤔

@ndelangen
Copy link
Member

I think we can use this:
https://jestjs.io/docs/en/configuration#modulenamemapper-object-string-string

@vercel vercel bot temporarily deployed to staging October 7, 2019 10:20 Inactive
@ndelangen
Merge branch 'next' into pr/stof/8266
10f84b0 
# Conflicts:
#	lib/components/package.json
@ndelangen ndelangen merged commit 36eea4b into storybookjs:next Oct 7, 2019
@stof stof deleted the upgrade_syntax_highlighter branch October 7, 2019 12:48
@shilman shilman mentioned this pull request Oct 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndelangen ndelangen ndelangen approved these changes

@igor-dv igor-dv Awaiting requested review from igor-dv

@tmeasday tmeasday Awaiting requested review from tmeasday

Assignees

@stof stof

@ndelangen ndelangen

Labels
dependencies
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@stof @ndelangen @shilman @phated