Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Rearchitect site for npm publishing #39

Merged

Conversation

barrymcgee
Copy link
Contributor

@barrymcgee barrymcgee commented Nov 3, 2017

Done

After digging into a little bit, I realised that the structure scaffolded out from create-react-app is not optimal for publishing a component library to NPM.

[1] facebook/create-react-app#907 (comment)

On the same thread, I noted where a team had forked create-react-app however for exactly this purpose so I've pulled out the work I've completed up until now and dropped it into that structure instead with Percy and Storybook.

Everything looks in order and the lib now builds as I'd expect and I can publish to npm, however, the app does not seem to be exporting modules correctly as when I try to import it and use a Button component, this happens:

https://codesandbox.io/s/03jv971v3n

Any ideas @anthonydillon or @Lukewh ?

Fixes #40

@webteam-app
Copy link

No ./run found. Unable to start demo.

@WillMoggridge
Copy link
Contributor

WillMoggridge commented Nov 6, 2017

@barrymcgee Your published package doesn't have a build/index.js

package.json:
"main": "build/index.js",

Checking package:

> ls node_modules/vanilla-framework-react/build/index.js                                            
ls: cannot access 'node_modules/vanilla-framework-react/build/index.js': No such file or directory

@WillMoggridge
Copy link
Contributor

The build created appears to be a compiled version of the site (I'm not sure what the site is in this case).

I don't think we want to upload this to NPM.
In react-bootstrap, the main entrypoint is set to lib/index.js. I think we can follow this pattern. The build for publishing to npm should probably just be the readme, license, package.json and the lib folder.

https://github.com/react-bootstrap/react-bootstrap/blob/master/package.json

This could be changed after this PR to unblock others. In that case I am happy with this.

@anthonydillon
Copy link
Contributor

@WillMoggridge is this ready for another review? If so, please update the labels.

Copy link
Contributor

@WillMoggridge WillMoggridge left a comment

Choose a reason for hiding this comment

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

Happy to merge this and iterate on it

Copy link
Contributor

@anthonydillon anthonydillon left a comment

Choose a reason for hiding this comment

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

Few questions in the code

"@storybook/addon-links": "^3.2.13",
"@storybook/addon-info": "^3.2.14"
},
"version": "0.0.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping the version from 0.1.0 to 0.0.5?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine, that is the current version on NPM and 0.0.x may be a better indication of the project.

@@ -1,6 +1,6 @@
import React from 'react';
import PropTypes from 'prop-types';
import './Accordion.css';
//import './Accordion.css';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be commented out?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should not be commented and explicitly load the CSS.

@WillMoggridge WillMoggridge merged commit ceace29 into canonical-web-and-design:master Nov 8, 2017
@barrymcgee barrymcgee deleted the publish-to-npm branch November 13, 2017 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rearchitect site for npm publishing
4 participants