Skip to content

Conversation

@a-tarasyuk
Copy link
Contributor

No description provided.

@btraut
Copy link
Contributor

btraut commented Jan 2, 2019

Looks like a reasonable change, but on code that really should be further updated -- asserts should never exist in production code. I recommend eventually following up by adding babel or some other build step to create both a dev and prod build of ReSub, and using that flag to conditionally strip out all asserts.

@deregtd
Copy link
Collaborator

deregtd commented Jan 2, 2019

My philosophy here is that anyone USING these sorts of libraries should simply implement babel if they want truly production-ready code, and use that to remove assets, minify, etc. Out of the box, for simpler projects, leave the asserts so people know when they've messed up. Anyone doing any real production-level code should be implementing a full release pipeline anyway.

@a-tarasyuk
Copy link
Contributor Author

@btraut thanks for your feedback., This PR is the first step to move away from cjs-assert, so I think changes related to the build process should be done in a new PR if these changes are necessary.

@deregtd Some of the optimizations can be tricky, like with lodash and could add an additional headache for users., As you know, I did investigations which shows that using entire lodash import in a library is bad, and the bundle will include entire lodash even with babel plugins which users apply to production code. Anyway, I think, would be helpful to add comments about some useful plugins which users have to apply in order to avoid issues related to bundle size.

@deregtd
Copy link
Collaborator

deregtd commented Jan 2, 2019

They can certainly be tricky, and pointing out how people should fix their build systems is certainly helpful. But I don't want to, say, remove all assert calls from our dist .js files on the public NPM repo for ReSub just to save public users a tiny bit of space. Lodash pre-optimizations don't cause our users any hassles, and it just saves them space with no real downside, but pre-optimizing-out assert calls hurts people.

@btraut
Copy link
Contributor

btraut commented Jan 2, 2019

Problem with leaving optimizations such as removing asserts from external libraries to the exercise of the user is that it requires a lot of internal knowledge of those libraries and different treatments for each. Rather than naming it assert, you could've used a different name or used console logging instead of throwing errors. By breaking ReSub out into its own module rather than inlining it in your project, you're taking on the burden of its build pipe, and that includes optimizations.

I'm inclined to follow React's lead with this stuff: asserts and error logging should only ever be present at dev time. Errors in prod will be more cryptic, but they're going to be minified anyway.

Anyway, I agree that this is unrelated to this PR in particular.

@deregtd
Copy link
Collaborator

deregtd commented Jan 2, 2019

How do you release a library with dev and production versions? Is there an NPM standard for that? I'm okay differentiating and removing ourselves if there's some way to release multiple versions in a single package. I'm under the impression you can only release a single library with NPM, so for that single library you have to go to the lowest common denominator (including asserts.)

@a-tarasyuk
Copy link
Contributor Author

If we are talking about React - it has several versions. By default, React will be in development mode. On NPM it completely relies on NODE_ENV, users have to worry about setting the environment in bundlers which they use, and nothing more.

index.js 

if (process.env.NODE_ENV === 'production') {
  module.exports = require('./cjs/react.production.min.js');
} else {
  module.exports = require('./cjs/react.development.js');
}

Also, there is opportunity to use UMD.

In the production version, it replaces errors with one error.

'Minified React error #' +
      code +
      '; visit %s ' +
      'for the full message or use the non-minified dev environment ' +
      'for full errors and additional helpful warnings. ',

For instance, Redux provides development/production versions only for UMD. Redux has ES2015 and CJS versions which have only one version which exists on NPM.

If it is necessary we can return to the discussion which I closed some time ago on SimpleRestClients, where I migrated library on Rollup + Babel to automate the build process.

@a-tarasyuk a-tarasyuk force-pushed the feature/remove-cjs-assert branch from 55386ba to 2c9f2ac Compare January 5, 2019 17:37
@berickson1 berickson1 merged commit 8be3596 into microsoft:master Jan 9, 2019
@a-tarasyuk a-tarasyuk mentioned this pull request Jan 11, 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.

4 participants