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

Feature/as library usage #256

Merged
merged 41 commits into from
Nov 4, 2021

Conversation

Lenivaya
Copy link
Collaborator

@Lenivaya Lenivaya commented Oct 21, 2021

Updates

Publishing

For publishing purposes, there are some changes in architecture based on rendering responsibility (gui used as library isn’t supposed to do any rendering things, just export components/gui as a widget)

  • index.ts is a library starting point responsible for exporting data-story as a widget without rendering it

  • render.ts is side-effect responsible for rendering application and bundling it to the public folder (won’t be included in published library)

Some other changes described below

Modules system

GUI is being built with both esm and cjs support

Styles

Styles are being divided on such groups

  • app (minimal css for displaying gui)
  • fonts (css for including fonts)
  • icons (css for including icons)

Fonts / icons

  • fontawesome icons now controlled via npm package
  • will be included in published library as optional css

Types

Included in published library

Imports from core

Related to data-story-org/core#149

For better backward compatibility we're using direct imports instead of imports from pre-defined subpaths, so it’s possible to use gui with older versions of bundlers.

Dependencies

  • dependencies needed only for development moved to devDependencies
  • react specified as a peerDependency, so no dependency collision when gui is used in another project

Misc

Styles

  • diagram has dynamic width
  • modals has little transition effects for opening and closing

appName, appDesc

Now that properties are constants in code, so no need to pass them via global window variable

Notes

react-diagrams and React.StrictMode

For some reasons usage of react StrictMode with react-diagrams in development mode throws errors. As this is not direct gui problem it’s nice to mention somewhere that disabling StrictMode must fix that problem.

projectstorm/react-diagrams#598 (comment)

Release workflow

Needs specified NPM_TOKEN in repo secrets to publish gui

Example of usage gui as library in another react app

Here’s how gui can be imported and used

import { DataStory } from "@data-story-org/gui";
import "@data-story-org/gui/lib/styles/app.css";
import "@data-story-org/gui/lib/styles/icons.css";
import "@data-story-org/gui/lib/styles/fonts.css";

return (
    <DataStory />
)
OKOOWDx.mp4

don't set explicit width
So when used as library gui can't break projects that still depends on
older webpack or react-scripts versions that can't handle subpath
exports defined in package.json
- index.ts is responsible for exporting data-story as a widget
without rendering it

- render.ts is side-effect responsible for rendering application and
bundling it to the public folder (won't be included in published library)
@Lenivaya Lenivaya requested a review from ajthinking October 21, 2021 21:55
@ajthinking
Copy link
Contributor

ajthinking commented Oct 22, 2021

Doing it like this:

npx create-react-app test2
cd test2
yarn add file:../gui
yarn start
then adding component
import { DataStory } from "@data-story-org/gui";
import "@data-story-org/gui/lib/styles/app.css";
import "@data-story-org/gui/lib/styles/icons.css";
import "@data-story-org/gui/lib/styles/fonts.css";


function App() {
return (
  <div className="App">
  		<DataStory />
  </div>
);
}

export default App;

Still gives that error. See: https://github.com/ajthinking/test2. Can you run yarn && yarn start and see in browser on your machine?

@Lenivaya
Copy link
Collaborator Author

Still gives that error. See: https://github.com/ajthinking/test2. Can you run yarn && yarn start and see in browser on your machine?

All works as it should for me, just add core to dependencies

"@data-story-org/core": "latest",

2021-10-23_00-15-51

If you still have some problems try re-initialize node_modules for gui also

@ajthinking
Copy link
Contributor

All works as it should for me, just add core to dependencies

Why would I need that? gui already has core as its own dependency?

Tried adding core as a dep, nuking gui and test apps node modules, same error.

Can you show your sample app?

@Lenivaya
Copy link
Collaborator Author

Tried adding core as a dep, nuking gui and test apps node modules, same error.

It's pretty strange because exactly the same repo with just a couple of changes in package.json works as shown in my previous commentary

"@data-story-org/core": "latest",
"@data-story-org/gui": "file:../gui",

Can you show your sample app?

It doesn't differ much from the default react template used in test 2 repo,

gui_embedding.zip

@ajthinking
Copy link
Contributor

gui_embedding.zip gives me same error, once I click to render <DataStory>
I have fresh gui on branch feature/as_library_usage, its up to date, freshly yarn build. Anything else I can try? 😅

@Lenivaya
Copy link
Collaborator Author

Anything else I can try?

Well as far as I can tell the problem must be in dependency collision. Can you run this command in react app and give the output?

npm list react

There will most likely be an extraneous react version which causes that error

@ajthinking
Copy link
Contributor

Yes!

image

I tested removing react from gui/node_modules, got me a bit further, then:

image
at
image

@Lenivaya
Copy link
Collaborator Author

Yes!

image

So without node_modules folder in gui (as it supposed to be when app is published as source code in packages registry) all must work without problems

image

Looks like some artifacts from old versions of gui when stories were saved in different format without circular data support. Cleaning up storage for the localhost:3000 must fix that error

@ajthinking
Copy link
Contributor

Cleaning up storage for the localhost:3000 must fix that error

Yes!! Works perfectly now! Thanks for the guidance

Copy link
Contributor

@ajthinking ajthinking left a comment

Choose a reason for hiding this comment

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

comments

I see we can do <DataStory appElement={'body'} /> now, this is great, probably something we want to continue on to allow user to pass more configurations, like which server to use or even their own server implementation could be passed there, which then would go into the rootStore.

gui build

Lots of warnings like

  ╷
6 │   font-size: (4em / 3);
  │               ^^^^^^^
  ╵
    node_modules/@fortawesome/fontawesome-free/scss/_larger.scss 6:15     @import
    node_modules/@fortawesome/fontawesome-free/scss/fontawesome.scss 8:9  @import
    src/sass/icons.scss 1:9                                               root stylesheet

: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

css files

Any reason why we cant/shouldnt provide one file for everything?

import "@data-story-org/gui/lib/styles/app.css";
import "@data-story-org/gui/lib/styles/icons.css";
import "@data-story-org/gui/lib/styles/fonts.css";

@Lenivaya
Copy link
Collaborator Author

Lots of warnings

: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

Must be fixed with a new version of fontawesome FortAwesome/Font-Awesome#17940

Any reason why we cant/shouldnt provide one file for everything?

The idea was to give user abillity to use his own copies or even different fonts, but for now yes it can be minimized to just one app css

@ajthinking
Copy link
Contributor

Must be fixed with a new version of fontawesome FortAwesome/Font-Awesome#17940

👍

The idea was to give user abillity to use his own copies or even different fonts

Nice idea, lets remember it for later if needed

@ajthinking
Copy link
Contributor

Fixes #232

@ajthinking
Copy link
Contributor

@Lenivaya this ready to merge?

@ajthinking
Copy link
Contributor

Added NPM_TOKEN in repository settings, as well as NPM_PUBLISH_TOKEN_CREATED_30_10_2021 in the organisation. They are the same secret in npmjs.org

@Lenivaya Lenivaya merged commit 35b093e into data-story-org:master Nov 4, 2021
@Lenivaya Lenivaya deleted the feature/as_library_usage branch November 4, 2021 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants