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

Make CanJS compatible with how webpack strips out dev code #4011

Closed
chasenlehara opened this Issue Mar 7, 2018 · 5 comments

Comments

Projects
None yet
6 participants
@chasenlehara
Copy link
Member

chasenlehara commented Mar 7, 2018

TLDR: use process.env.NODE_ENV !== 'production' in CanJS’s code to support how other bundlers (like webpack) remove code from production builds.

This was discussed on a recent live stream (34:36) and a previous live stream (18:28).

Overview

Right now, CanJS relies on steal’s removeDevelopmentCode build option to remove code when built for production:

//!steal-remove-start
  // Code removed from the production build
//!steal-remove-end

This doesn’t work out of the box with other bundlers, which support something like this (Browserify, webpack):

if (process.env.NODE_ENV !== 'production') {
  // Code removed from the production build
}

We can start using the above in our code today by using steal’s envify build option.

Unfortunately, replacing our proprietary //!steal-remove-* comments with process.env.NODE_ENV checks would cause existing steal users to start having debug code in their production build, so until CanJS 5, I think we would need to have both in our source code like this:

//!steal-remove-start
if (process.env.NODE_ENV !== 'production') {
  // Code removed from the production build
}
//!steal-remove-end

This is potentially codemod-able, but because we sometimes use the steal comments inside objects, like this:

funcCall({
  //!steal-remove-start
  debugArg: true
  //!steal-remove-end
})

…I think it would be better to just manually make the change everywhere. At the time of this writing, there are 139 results for steal-remove-start in the CanJS org, so I think it would take about a week for someone to update all the code and release it.

How we teach this

After we make this change, I think we should add a new “Production” topic to our guides that explains how to build for production with popular bundlers. Alternatively, we could have a more generic “Performance” page that includes this and other performance tips for CanJS as a whole.

Additionally, we should consider adding info about this and using can-log to our contribution guide, so contributors know how to correctly add useful debugging info that’s removed in production.

Drawbacks & Alternatives

If this proposal is accepted “now” (Spring 2018), I think we should include both the steal comments and process.env checks, although this makes our code more difficult to maintain now and would suggest work in the future to remove the steal comments in the next major versions of each package.

Alternatively, we could wait to replace the steal comments with process.env checks in the next major version of each package, although that wouldn’t help packages like can-util in which we’re intentionally trying to avoid a new major version.

Unresolved questions

Are there other alternatives or workarounds that I didn’t include in this proposal?

@matthewp

This comment has been minimized.

Copy link
Contributor

matthewp commented Mar 7, 2018

You could wrap your conditionals with the comments :/

@chasenlehara chasenlehara changed the title Make CanJS compatible with how other bundlers strip out dev code Make CanJS compatible with how webpack strips out dev code Mar 7, 2018

@r0m4n

This comment has been minimized.

Copy link

r0m4n commented Jun 6, 2018

Does anyone know if there's any workaround for removing these dev warnings when bundling with something other than steal? For example, browserify

@justinbmeyer

This comment has been minimized.

Copy link
Contributor

justinbmeyer commented Jun 6, 2018

You could add the same logic that steal-tools uses as a pre-build hook. Alternately, you could help us migrate some of these blocks over to a compatible format.

@cherifGsoul

This comment has been minimized.

Copy link
Member

cherifGsoul commented Jun 22, 2018

Here is a list to track changes in modules for the first part of the issue, the checked means is approved and merged:

  • can-ajax
  • can-assign
  • can-attribute-encoder
  • can-attribute-observable
  • can-bind
  • can-component
  • can-compute
  • can-connect
  • can-connect-feathers
  • can-connect-tag
  • can-construct
  • can-construct-super
  • can-control
  • can-data-types
  • can-debug
  • can-define
  • can-define-backup
  • can-define-lazy-value
  • can-define-stream
  • can-define-stream-kefir
  • can-define-validate-validatejs
  • can-deparam
  • can-diff
  • can-dom-data
  • can-dom-data-state
  • can-dom-events
  • can-dom-mutate
  • can-event-dom-enter
  • can-event-dom-radiochange
  • can-event-queue
  • can-fixture
  • can-fixture-socket
  • can-globals
  • can-kefir
  • can-key
  • can-key-tree
  • can-list
  • can-local-store
  • can-log
  • can-make-map
  • can-map
  • can-map-define
  • can-memory-store
  • can-namespace
  • can-ndjson-stream
  • can-observation
  • can-observation-recorder
  • can-observe
  • can-param
  • can-parse-uri
  • can-query-logic
  • can-queues
  • can-realtime-rest-model
  • can-reflect
  • can-reflect-dependencies
  • can-reflect-promise
  • can-reflect-tests
  • can-rest-model
  • can-route
  • can-route-pushstate
  • can-set-legacy
  • can-simple-dom
  • can-simple-map
  • can-simple-observable
  • can-stache
  • can-stache-bindings
  • can-stache-converters
  • can-stache-key
  • can-stache-route-helpers
  • can-stream
  • can-stream-kefir
  • can-string
  • can-string-to-any
  • can-super-model
  • can-symbol
  • can-test-helpers
  • can-util
  • can-validate
  • can-validate-interface
  • can-validate-legacy
  • can-validate-validatejs
  • can-vdom
  • can-view-autorender
  • can-view-callbacks
  • can-view-live
  • can-view-model
  • can-view-nodelist
  • can-view-parser
  • can-view-scope
  • can-view-target

@cherifGsoul cherifGsoul self-assigned this Jun 26, 2018

@cherifGsoul cherifGsoul added the review label Jun 28, 2018

chasenlehara added a commit to canjs/can-value that referenced this issue Jun 29, 2018

chasenlehara added a commit to canjs/can-simple-observable that referenced this issue Jun 29, 2018

chasenlehara added a commit to canjs/can-stache-bindings that referenced this issue Jun 29, 2018

@cherifGsoul

This comment has been minimized.

Copy link
Member

cherifGsoul commented Jul 5, 2018

@chasenlehara all listed modules were fixed I think we can close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.