Skip to content
This repository was archived by the owner on Jan 21, 2019. It is now read-only.

Conversation

vforge
Copy link
Collaborator

@vforge vforge commented Sep 26, 2018

  • Move all the source files to source directory.

  • Update READMEs everywhere.

  • For now (until we have private npm account or artifactory) we cannot publish the package, so the output files need to stay in this repo.

  • I do not want the client to download X libraries that are not needed in order to use the RDS (like Ember is needed for the DS itself), so the process is compiling all the files into ES5 and you only need React, ReactDOM and PropTypes in order to use them. The client shouldn't care if an RDS component needs lodash or anything else.

  • For the reason above I do not want to have a post-install process. In the future, the build should happen only before the package is published.

NOTE: This PR does not contain build files, because I wanted it to be easier to read. Once it's clear, I'll add the result of yarn build.

@vforge vforge added the WIP label Sep 26, 2018
@coveralls
Copy link

coveralls commented Sep 27, 2018

Pull Request Test Coverage Report for Build 431

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 411: 0.0%
Covered Lines: 250
Relevant Lines: 250

💛 - Coveralls

@vforge vforge removed the WIP label Sep 27, 2018
@JoshuaRogan
Copy link
Contributor

I think nearly all of these files should be generated on prepublishOnly or something similar. It makes it very hard to write code with all of this extra code.

@@ -0,0 +1,366 @@
'use strict';

function _interopDefault (ex) { return (ex && (typeof ex === 'object') && 'default' in ex) ? ex['default'] : ex; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is all this extra code auto-generated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes!

@mklucsarits
Copy link
Contributor

@v Can you explain a bit more what's going on here? Like what the extra code is doing and how it's generated?

@vforge
Copy link
Collaborator Author

vforge commented Sep 28, 2018

@JoshuaRogan Unfortunately because of our licensing we won't publish this package to npm.js. There are other ideas floating around, but for now everything stays on GH.

@mklucsarits Sure, let me add READMEs and some description during the weekend and we'll get back to this PR next week.

@vforge vforge force-pushed the new-build-process branch 2 times, most recently from d3e0af0 to 2fc7c01 Compare October 1, 2018 18:36
@vforge vforge force-pushed the new-build-process branch from 2fc7c01 to 2f330b1 Compare October 1, 2018 18:53
@JoshuaRogan
Copy link
Contributor

JoshuaRogan commented Oct 1, 2018

I believe you can run it during npm install without it being on npm. https://docs.npmjs.com/misc/scripts

@vforge
Copy link
Collaborator Author

vforge commented Oct 1, 2018

Don't use install. Use a .gyp file for compilation, and prepublish for anything else. You should almost never have to explicitly set a preinstall or install script. If you are doing this, please consider if there is another option. The only valid use of install or preinstall scripts is for compilation which must be done on the target architecture.

https://docs.npmjs.com/misc/scripts#best-practices

@vforge
Copy link
Collaborator Author

vforge commented Oct 1, 2018

I'll in the process of updating docs/readmes so it's clear why it's like that right now.

@vforge vforge force-pushed the new-build-process branch 4 times, most recently from 5c7355d to 04c4dcd Compare October 2, 2018 17:27
@vforge vforge force-pushed the new-build-process branch from 04c4dcd to 9b07625 Compare October 2, 2018 17:36
@vforge
Copy link
Collaborator Author

vforge commented Oct 2, 2018

@mklucsarits and @JoshuaRogan I've updated PR and added a description. Hopefully, it's clearer now.

mklucsarits
mklucsarits previously approved these changes Oct 2, 2018
@vforge
Copy link
Collaborator Author

vforge commented Oct 2, 2018

@mklucsarits automated dismiss, sorry about that!
screenshot 2018-10-02 15 38 56

I turned it off

@mklucsarits mklucsarits merged commit aa21694 into master Oct 2, 2018
@mklucsarits mklucsarits deleted the new-build-process branch October 2, 2018 22:57
@JoshuaRogan
Copy link
Contributor

@vforge Can we use artifactory to host the content? It really sucks having the build files in the repo.

@vforge
Copy link
Collaborator Author

vforge commented Oct 12, 2018

@JoshuaRogan I'll work on that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants