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

Improve compatibility with webpack #4170

Closed
chasenlehara opened this Issue May 31, 2018 · 22 comments

Comments

Projects
None yet
7 participants
@chasenlehara
Member

chasenlehara commented May 31, 2018

TLDR: improve webpack compatibility by making sure debug code gets removed in production builds, contributing to a webpack loader for stache templates, and adding tests for loading our modules.

This was discussed on a recent live stream (25:22).

There are three main problems with using CanJS & webpack right now. This proposal is to fix all three issues.

Debug code is left in production builds

This problem & its solution is covered in Make CanJS compatible with how webpack strips out dev code.

Loading stache templates is unintuitive

In our docs for setting up CanJS & webpack, we encourage people to install the raw-loader and use it like so:

import template from "raw-loader!./app.stache";

Instead, I propose that we contribute to can-stache-loader and show it in our guides. That package currently supports CanJS 2.3 and has a pre-release with 3.x support; I propose that we contribute 4.x compatibility and help maintain it in the future.

If @macku doesn’t want our help maintaining the project (I hope you do Maciej!), then we can create our own package to accomplish the same goals.

We should also update our setting up CanJS & webpack docs to show the loader; this should include updating our webpack-example repo. Additionally, we should submit a PR to include the loader in the webpack Loaders docs.

Sometimes our modules can’t be loaded by webpack

Every once in a while, we make mistakes that work fine with StealJS but not with webpack.

To help catch these mistakes sooner, I propose that we add tests to the main CanJS test suite that load all the modules with webpack. At a minimum, this would help us ensure that everything can be imported/required.

Alternatively, we could do this in each package and run the test suite to make sure everything works as expected, but I think this would be too much of a maintenance burden for too little reward.

@macku

This comment has been minimized.

macku commented Jun 5, 2018

Hey @chasenlehara! I'm happy to help you. I've created the webpack loader some time ago for one of the projects I was involved in the past. If you feel you would like to contribute to the project or even take an ownership of it I could grant you all needed accesses.

@rjgotten

This comment has been minimized.

rjgotten commented Jun 12, 2018

@chasenlehara
Offering a potential alternative: I've been working on a company-internal webpack loader for stache, compatible with CanJS 4. Seems like feature-wise it's a bit more mature than the work @macku did with his loader sofar.

Ours has support for extracting and processing nested {{> ./partial.stache}} partials into the webpack bundle as well as nested imports that use <can-import> -- the latter including support for pulling the imported value into a scope variable using <can-import value:to="scope.vars.(...)" />.

(As an aside: It's quite tricky to get this working with webpack's interpretation of require(), since it likes everything neatly defined upfront in a nice-and-static way. I had to have the loader resort to injecting a redefinition of the <can-import> tag, which avoids the can.import method on which the normal tag relies, since that's where a dynamic import strategy is used.)

Ours also generates webpack-compatible source maps and handles them quite nicely: It creates a sub-folder for each foo.stache file, which holds a foo.js source file containing the generated code -- incase you ever need to set breakpoints -- and a foo.html file containing the original template -- for reference during debugging. (No setting breakpoints inside the template. As wicked cool as it would be to get that working, it is probably outside the realm of possibility right now.)
The .html extension may seem weird, but it's necessary for browser developer tools to not spaz out and report syntax errors, atleast until the source maps specs are updated to allow for content-type information and the tools start recognizing it.

If you're interested, we could probably donate the code. Would have to double-check with management, but since it's not exactly core business related material, it'll probably be fine.

@justinbmeyer

This comment has been minimized.

Contributor

justinbmeyer commented Jun 19, 2018

As part of this issue, we will need to make sure the build actually works. I think we are currently testing the build. We will need to test this fashion of build too.

Should we do this for both 4.X and 5.X?

@matthewp

This comment has been minimized.

Contributor

matthewp commented Jun 19, 2018

I created a smoke test that has a basic app that's built with webpack. It's here: https://github.com/canjs/canjs/tree/major/test/builders/webpack

Ideally we would run all of the canjs tests through webpack, but many of them depend on steal.

@cherifGsoul

This comment has been minimized.

Member

cherifGsoul commented Jun 19, 2018

I started working on can-stache for this and made a branch https://github.com/canjs/can-stache/tree/fix-debug-webpack-style it needs webpacks test suite!

@rjgotten

This comment has been minimized.

rjgotten commented Jun 20, 2018

@chasenlehara

Quickly tossed together a repository that holds an export of our current code. Feel free to have a look.
https://github.com/NetMatch/webpack-stache-loader

@cherifGsoul

This comment has been minimized.

Member

cherifGsoul commented Jun 20, 2018

@chasenlehara when you said I propose that we add tests to the main CanJS test suite that load all the modules with webpack you mean loading test modules?

@chasenlehara

This comment has been minimized.

Member

chasenlehara commented Jun 20, 2018

What I had in mind was making sure require("can-component") or import "can-component" worked correctly with webpack. This would help us avoid simple mistakes like steal-specific syntax or having the wrong main in package.json.

I’m not exactly sure what a test for this should look like… maybe it would require building a bundle with webpack? Maybe this is already covered by the tests @matthewp added for #4200 or #4133?

@cherifGsoul

This comment has been minimized.

Member

cherifGsoul commented Jun 20, 2018

Should we write tests that check that warning and debug messages ar e not displayed in the production builds?

@justinbmeyer

This comment has been minimized.

Contributor

justinbmeyer commented Jun 21, 2018

I think we should have a "webpack-test" module that imports all the tests that are "safe" for webpack to run. Then we should have webpack build those tests and run the built version.

By "safe", I mean tests that are not using steal-clone and similar special features.

@matthewp

This comment has been minimized.

Contributor

matthewp commented Jun 21, 2018

Currently there are probably no safe modules, given that all of the canjs tests use steal-qunit. Would first need to create a clone of that for webpack.

@rjgotten

This comment has been minimized.

rjgotten commented Jun 21, 2018

having the wrong main in package.json.

FYI; the resolution taken to that issue runs counter towards common practice.
"main" should point at dist/. Otherwise you are forcing all your consumers to set up their own transpilation, not just their own code but also selectively for certain of their modules installed in node_modules. Even if they didn't even need transpilation to begin with.

Misunderstandings usually stem from people confusing dist/ with "meant for browsers," which it is not.

A correct "works everywhere" set-up is the following:

  1. Have src/ contain non-transpiled ES6 code that can use features even not supported by Node.js
  2. Transpile src/ to dist/ with a target for Node.js
  3. Transpile src/ to browser/ with a target for browsers
  4. Point main in package.json at the entry-point in dist/
  5. Point browser in package.json at the entry-point in browser/
  6. Add dist/ and browser/ to .gitignore
  7. Add src/ to .npmignore

Many bundlers including Webpack can pick their compile target, switching between e.g. node and web, and will respect the browser field from package.json while picking the entry point for the module.

Finally, in the case of modern bundlers that understand ES6 modules and can perform tree-shaking, you'll want a build that transpiles everything down to ES5, except the ES6 import statements. You can put that in module/ and point the module property of package,json at it.

Up-to-date bundlers like Rollup and Webpack should respect that property as well.

A decent reference on the topic

@matthewp

This comment has been minimized.

Contributor

matthewp commented Jun 21, 2018

@rjgotten There's nothing that needs to be transpiled in canjs packages. The src/ contains plain CommonJS written in ES5. The reason why our main is some times wrong is because the projects were created with donejs add plugin foo which sets the main to dist/cjs/foo.js, but we don't need to transpile to commonjs.

@cherifGsoul

This comment has been minimized.

Member

cherifGsoul commented Jun 21, 2018

@matthewp can you please tell me what do you mean by "clone"? write the same tests for webpack?

@matthewp

This comment has been minimized.

Contributor

matthewp commented Jun 21, 2018

I mean that you would need to write a webpack plugin (which I don't know how to do) that makes it possible to include steal-qunit, since all of our tests use steal-qunit.

I would probably wait on doing this. It would be quite a task. I would just focus on adding the if(process.env.NODE_ENV !== "production) { stuff.

@cherifGsoul

This comment has been minimized.

Member

cherifGsoul commented Jun 21, 2018

@matthewp thank you, that's what I'm doing now, I just want to have some ideas on how tests could be written.

This was referenced Jun 25, 2018

@phillipskevin

This comment has been minimized.

Collaborator

phillipskevin commented Jul 5, 2018

@cherifGsoul @chasenlehara can this be closed?

@justinbmeyer

This comment has been minimized.

Contributor

justinbmeyer commented Jul 5, 2018

We need webpack-stache to fully close it I think.

@cherifGsoul

This comment has been minimized.

Member

cherifGsoul commented Jul 5, 2018

@phillipskevin this what should be closed for fixning dev mode!

@matthewp

This comment has been minimized.

Contributor

matthewp commented Jul 10, 2018

@macku Would you mind transferring your project to the canjs org?

@chasenlehara

This comment has been minimized.

Member

chasenlehara commented Jul 10, 2018

@matthewp I reached out to him over email. 😊

@macku

This comment has been minimized.

macku commented Jul 11, 2018

@matthewp @chasenlehara I've granted you a permission to the NPM package. Let me check what I can do with github repo.

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