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

Publish ES5 sources to NPM, get react-native-web compat for free #173

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

emirotin
Copy link

@emirotin emirotin commented Nov 1, 2017

I notice it's common for RN libs to be published in ES6+ and often with flow types. While this is fine with RN itself it's uncommon for the web-targeting modules which are supposed to be published in the common denominator format.

Specifically if one is using create-react-app (which is very popular of course) node_modules are not processed by babel and are only packaged by webpack. So certain features like flow types, or spread destructuring, or JSX in case of this module make the build break.

Now, why would one need RN lib in the web / with CRA? Because of "React Native for web" project.
I'm currently working on an Electron app with RN-web and would like to later reuse the same components for the actual RN mobile app.

So I suggest following the common pattern of publishing the compiled ES5 sources to NPM. I've also slightly updated the dependencies and simplified the Prettier+ESLint config following the recommendations from the Prettier docs.

I've also tested my fork with my current project and it works:
screenshot

@emirotin
Copy link
Author

emirotin commented Nov 1, 2017

I will also put some comments/annotations to the sources now.

Copy link
Author

@emirotin emirotin left a comment

Choose a reason for hiding this comment

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

Annotated the changes

"presets": ["react-native"]
"presets": [
[
"env",
Copy link
Author

Choose a reason for hiding this comment

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

This is the modern replacement fro es2015 preset. It autodetects the features to be transpiled / polyfilled based on the target platforms.

example
lib
Copy link
Author

Choose a reason for hiding this comment

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

Built sources should not be linted

module.exports = {
"extends": ["eslint:recommended", "plugin:react/recommended"],
extends: ['eslint:recommended', 'plugin:react/recommended', 'prettier'],
Copy link
Author

Choose a reason for hiding this comment

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

All the formatting was done by Prettier. It can be restored and file added to ignores if you wish.

"no-console": 0,
"no-shadow": 2,
"no-var": 2,
// style
Copy link
Author

Choose a reason for hiding this comment

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

No need for style rules — we're using Prettier so can rely on its own rules set. The 'prettier/prettier': 'error', ensures linting will fail if there's contradiction with prettier's formatting.

@@ -4,7 +4,7 @@ import renderer from 'react-test-renderer';

import {StyleSheet, Text} from 'react-native';

import HTMLView from '../HTMLView';
import HTMLView from '../lib';
Copy link
Author

Choose a reason for hiding this comment

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

Test the built sources instead of originals to also capture possible transpilation issues

@@ -2,23 +2,31 @@
"name": "react-native-htmlview",
"version": "0.12.1",
"description": "A component which renders HTML content as native views",
"main": "index.js",
"jsnext:main": "src/index.js",
"main": "lib/index.js",
Copy link
Author

Choose a reason for hiding this comment

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

This is for "regular" consumers

"yarn run format-lint && yarn run lint && yarn run babel && yarn run jest",
"format-lint":
"prettier --list-different \"./**/*.js\"; if [ $? != 0 ]; then echo \"CODE FORMATTING: please run 'yarn run format' and commit the changes\"; exit 1; fi",
"format": "prettier --write \"./**/*.js\" && eslint . --fix",
Copy link
Author

Choose a reason for hiding this comment

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

We can actually drop the eslint part now

"format": "./format.sh --write && eslint . --fix",
"jest": "jest"
"test":
"yarn run format-lint && yarn run lint && yarn run babel && yarn run jest",
Copy link
Author

Choose a reason for hiding this comment

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

Build sources before testing

"format": "prettier --write \"./**/*.js\" && eslint . --fix",
"jest": "jest",
"babel": "babel src --out-dir lib --source-maps",
"prepublishOnly": "yarn run test"
Copy link
Author

Choose a reason for hiding this comment

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

Run tests (and build sources) before publishing

@@ -1,8 +1,5 @@
import React, {PureComponent} from 'react';
Copy link
Author

Choose a reason for hiding this comment

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

All sources moved to a separate folder to separate them from repo spam and simplify the babel command

@richchurcher
Copy link
Collaborator

Hi @emirotin ! Thanks for your thorough reworking of the build, lint and test process, and for your patience. There are a few moving parts in this PR and I want to sit with it for just a bit longer before I merge 😄 I will take another look end of day today or tomorrow.

People wondering about transpilation of RN modules may be interested in facebook/react-native#10966 .

@emirotin
Copy link
Author

emirotin commented Apr 5, 2019

Hey, I know it's been a while but I've now returned to the project I was using your lib for and with all the deps updated (react, react-native-web, react-scripts, etc.) it's still the same issue. So I've updated this PR, rebased it against your most recent master and squashed the technical commits. The thing works for me as expected so I suggest you reconsider this PR.
One thing that can be changed if you wish is excluding the lib/ from git as it's the derived artifact that can always be built from sources before testing and before publishing to NPM. I need it right now though because I'm using this module directly from my git fork and I need it usable without the build step.

@@ -0,0 +1 @@
lib
Copy link
Author

Choose a reason for hiding this comment

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

Without this, the source maps would be broken.

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