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

Babel for es2015 enviornments #69

Merged
merged 6 commits into from May 13, 2019
Merged

Babel for es2015 enviornments #69

merged 6 commits into from May 13, 2019

Conversation

rjerue
Copy link
Contributor

@rjerue rjerue commented Mar 12, 2019

Hello,

Love this library! I am using it in an environment where dependencies must be es2015 though and need it run through babel. Jest was already running things through babel.

This is a very normal thing for Javascript libraries. Especially those that can be leveraged on web and native.

yarn release will create the necessary files in a folder called 'lib' ... it's a prepublish hook so when you run yarn publish it should just work.

Also upgraded Jest.

Tested on web and native, also passes jest tests.

@rjerue
Copy link
Contributor Author

rjerue commented Mar 12, 2019

Travis seems to not be a big fan of yarn -- will investigate tomorrow.

@coveralls
Copy link

coveralls commented Mar 12, 2019

Coverage Status

Coverage remained the same at 85.606% when pulling b5c1822 on rjerue:master into 58da94c on mfrachet:master.

@rjerue
Copy link
Contributor Author

rjerue commented Mar 12, 2019

Travis has been fixed. The code coverage change comes because of the index file being changed. Would love some feedback :)

Copy link
Owner

@mfrachet mfrachet left a comment

Choose a reason for hiding this comment

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

Thank you for this PR 😊 it's great to see new coming people!

I'm actually really busy working and I will try to check how it works and give you some feedbacks as soon as I can 😊

Again, thank you!

.travis.yml Outdated
@@ -1,5 +1,5 @@
language: node_js
node_js:
- "9"
- "stable"
Copy link
Owner

Choose a reason for hiding this comment

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

Do you know why bumping to the last stable one would make this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the modules in node_modules has a check to make sure the version of node being run is incompatible with node 9.

error sane@4.0.3: The engine "node" is incompatible with this module. Expected version "6.* || 8.* || >= 10.*".

I assume because this is they want you to be using a stable version, or one that is LTS. odd number releases are not LTS.

presets.push(['@babel/preset-env', { modules: false }]);
}
return { presets };
};
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think of:

module.exports = api => {
  const preset = api.env('test')
    ? 'module:metro-react-native-babel-preset'
    : ['@babel/preset-env', { modules: false }];

    return { presets: ['@babel/preset-react', preset]
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prefer it 👍

@rjerue
Copy link
Contributor Author

rjerue commented Mar 13, 2019

Thank you for this PR 😊 it's great to see new coming people!

I'm actually really busy working and I will try to check how it works and give you some feedbacks as soon as I can 😊

Again, thank you!

Busy over here too. Will be using it in my fork in the meantime. Will switch to upstream when merged in.

Cheers 🍻

@mfrachet
Copy link
Owner

Hi @rjerue ! I've released a v2.0.0 yesterday and wanted to know if you're still interested to work on this PR to integrate a build system to the lib?

Thanks for your answer 😊

@rjerue
Copy link
Contributor Author

rjerue commented Apr 28, 2019

Hi @rjerue ! I've released a v2.0.0 yesterday and wanted to know if you're still interested to work on this PR to integrate a build system to the lib?

Thanks for your answer 😊

@mfrachet Yes! I'll take a look when in the office on Monday.

@rjerue
Copy link
Contributor Author

rjerue commented Apr 29, 2019

@mfrachet This is now done. Also have changed the code coverage to ignore the added "index" file and the "lib" ones.

Copy link
Owner

@mfrachet mfrachet left a comment

Choose a reason for hiding this comment

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

Thanks for the job @rjerue 😊

.gitignore Outdated
@@ -52,3 +52,5 @@ buck-out/
fastlane/report.xml
fastlane/Preview.html
fastlane/screenshots

lib
Copy link
Owner

Choose a reason for hiding this comment

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

Well, I like to work and merge on master and to be able to test the bundles on this branch (or other branches) before publish on npm. So what do you think to leave the lib folder on the master branch?

I'm not sure it's a real problem. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not something I do as I don't like putting minified code onto git.

However, I think I've got a good idea for this. If we run the build as part of the precommit, that should rectify this as the latest build will always be in git.

As an added bonus, people can install from git as opposed to npm if they'd like.

@@ -1,5 +1,5 @@
language: node_js
node_js:
- "11"
- 'stable'
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for this 😄

import replace from 'rollup-plugin-replace';

const NODE_ENV = process.env.NODE_ENV || 'development';
const outputFile = NODE_ENV === 'production' ? './lib/prod.js' : './lib/dev.js';
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure to get why we need to rely on specific environements here? I'm not that good with building tools 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes, bundled code works differently if built for dev or production. As a result, I actually compile two binaries. Prod.js and Dev.js. Often, prod dependencies log nothing related to debugging.

Your code isn't actually the reason that I'm doing this, it's because of the dependencies which in this case is only prop-types. I'm unsure if proptypes (or any of it's dependencies) require this. It's often best practice to just put it in something that in. The rollup config I use on my FOSS projects is pretty standard.

@@ -31,9 +35,15 @@
"dependencies": {
"prop-types": "15.6.2"
},
"peerDependencies": {
"react": "^16.8.6"
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't we add react-native as a peer too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't add React-Native for the same reason we don't add react-dom.

React-Native-Web can be used instead, and npm will throw a warning every time an npm install is run.

@mfrachet
Copy link
Owner

I've took this open for too long. Thanks for the job you've made, I'm merging this and will check for further fix if I'm encountering problems 👌

@mfrachet mfrachet merged commit ee2c4e4 into mfrachet:master May 13, 2019
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.

None yet

3 participants