dojo / cli-build-widget

Command for building Dojo widgets
Other
6 stars 17 forks source link

Add `--target=lib` flag #42

Closed mwistrand closed 5 years ago

mwistrand commented 6 years ago

Resolves #35

Allow widget libraries to be built with --target=lib (or -t lib for short). With this flag, all TypeScript and CSS files beneath src/ are processed and output to output/{mode}, along with any font and/or image assets. Although this does not utilize webpack, the --legacy flag is still used to determine the CSS and TypeScript targets.

At the moment this is excluded from eject since it is entirely independent of the webpack build used to bundle custom elements, although at some point it would be worthwhile to determine the best way to handle such cases.

agubler commented 6 years ago

Also does this mean that building to the library target doesn't support watching (and maybe other options)? How is this going to work with cli-test-intern? As that would be the command we'd want to recommend for testing a dojo widget library?

mwistrand commented 6 years ago

@agubler I will look into adding a watch option as well as other flags available for the webpack build. As mentioned in the description, this does not yet work with eject, as it is independent of the webpack build, so we'd need provide a separate way to build libraries once ejected. I have this on my to-do list, but wanted to get this up for at least an initial review.

agubler commented 6 years ago

@mwistrand I should have read the description re eject really!

mwistrand commented 6 years ago

There are still a couple minor annoyances I need to resolve around logging (as well as an additional fix), but this is now compatible with --serve and --watch. As for compatibility with @dojo/cli-test-intern, that command expects fully-built output/test/tests/unit.js and output/test/tests/functional.js files to run, at least with dojo test -c local. The easiest solution is to require an index.ts file that can serve as the entry point for libraries, so that the webpack build could be used. Then again, that would result in testing something other than what is normally built with --target=lib.

agubler commented 6 years ago

@mwistrand coming together 👍 Would be great if the output was formatted like the output from the custom element target... Would that be possible?

mwistrand commented 6 years ago

@agubler After playing around with this a bit, I came up with what you see below. Note that we still need to determine how to best use this with cli-test-intern, and that the current expectation is that libraries will use separate src/ and tests/ directories (so this is not a drop-in replacement for the custom @dojo/widgets build, for example).

dojo-build-widget-tlib

codecov[bot] commented 5 years ago

Codecov Report

Merging #42 into master will decrease coverage by 6.17%. The diff coverage is 44.94%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #42      +/-   ##
=========================================
- Coverage   70.67%   64.5%   -6.18%     
=========================================
  Files           8       8              
  Lines         358     400      +42     
  Branches       59      83      +24     
=========================================
+ Hits          253     258       +5     
- Misses        105     142      +37
Impacted Files Coverage Δ
src/ejected.config.ts 100% <100%> (ø) :arrow_up:
src/util.ts 100% <100%> (ø) :arrow_up:
src/logger.ts 100% <100%> (ø) :arrow_up:
src/base.config.ts 24% <2.7%> (-9.83%) :arrow_down:
src/dist.config.ts 37.5% <27.27%> (-6.95%) :arrow_down:
src/dev.config.ts 33.33% <60%> (+2.56%) :arrow_up:
src/main.ts 95.31% <82.35%> (+0.03%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dc78b9e...2917832. Read the comment docs.

agubler commented 5 years ago

@mwistrand At the moment we can only build either .js (legacy) or .mjs (evergreen), we'll need a way to build both for publishing. What would the recommended way be to do that? For example, we cannot just do two builds because it cleans the output directory?

agubler commented 5 years ago

Also I think we'll want to minify the output css (we use cssnano currently in cli-build-app and dojo/widgets)

agubler commented 5 years ago

Could we configure the lib mode in the same way as we currently configure widgets to build as custom elements? It would be good if there was a consistent way to do this across the two modes. Perhaps we could rename the element dojorc configuration to something more generic like widgets and in ce mode it works as it does today, but in lib mode we generate the entry point for the user.

mwistrand commented 5 years ago

@agubler I've pushed changes that will 1) generate an entry point from the .dojorc and 2) output both legacy and evergreen side-by-side. Once tests are passing, I'll push my cssnano changes to my webpack-contrib PR.

agubler commented 5 years ago

Thanks @mwistrand! Let me know when it's ready for review again :)

agubler commented 5 years ago

One thing can we rename the elements option to widgets to cover both targets?

willstott101 commented 5 years ago

Are there any patches required in a cli-build-app powered app to use a library built with this?

In my dev build main.js I've ended up with a button.m.css which is imported into button.m.css.js which is used to resolve class names. However button.m.css.js never uses the map in button.m.css, so the resulting class names on the generated elements do not match the final concatenated css, only matching the class names in the output from this tool.

EDIT: I'm sorry if that's really unclear I'm a pretty lost in all this complex build stuff.

agubler commented 5 years ago

@mwistrand @dojo/webpack-contrib@6.0.0-alpha.1 has been released

mwistrand commented 5 years ago

@willstott101 If you're CSS is getting mangled then something is wrong; you shouldn't have to change anything to your app to use a library built with @dojo/cli-build-widget. For all intents and purposes, a library built with @dojo/cli-build-widget should look like any other library you might import into your application. Do you have a demo application (e.g., in codesandbox) you can share that displays this problem? In the meantime, I'll see if I can reproduce this issue on my end.

mwistrand commented 5 years ago

@willstott101 I haven't yet explored much more deeply than this, but the correct CSS classes are preserved when building @dojo/widgets with dojo build widget -t lib and using the resulting package with a default app created with dojo create app: https://github.com/mwistrand/dojo-widgets-target-lib

The deps/ directory includes both the widgets tarball, as well as a package/ directory that shows its contents, which were generated with dojo build widget -t lib (and then copying over the README and package.json).

Since @dojo/cli-build-app will only generate unique CSS classes for CSS files found within the src/, any CSS files found outside of src/ (for example, in node_modules/) should remain untouched. Please let me know if that is not your experience.

willstott101 commented 5 years ago

Thankyou very much for looking into this. It's quite possible I've got something wrong. I can't get at my work repos till Monday, but I can say that I'm trying to theme the imported widgets.

I'll try and use the widgets from my lib without themeing and see if the class names still change.

Should the names stay the same even if they're being themed by another (cli-build-theme) package?

agubler commented 5 years ago

@mwistrand Re the hashed assets like fonts, doesn’t everything reference the assets non hashed and by the original paths? How do the hashed asset names work?

agubler commented 5 years ago

@mwistrand Sorry I've given you conflicts again :( I took the serve changes also.

mwistrand commented 5 years ago

@agubler The file-loader reports the new name to the CSS loader(s) so the widget library files reference the correct filename. That said, since @dojo/cli-build-app would hash any assets anyway, I'll update to preserve the original filenames in their original directory locations, with the caveat that assets will be referenced from the project root (for example, src/theme/icon.m.css would import src/theme/fonts/dojo2.ttf with url(../theme/fonts/dojo2.ttf) instead of url(./fonts/dojo2.ttf)).

agubler commented 5 years ago

@mwistrand Okay, I think that's okay - I don't think I have any comments left! Have we tried it with @dojo/widgets?

mwistrand commented 5 years ago

@agubler I’ve been using @dojo/widgets as my guinea pig for these changes, which I’ve then imported into a test application. For a sanity check I’ll test with the widget showcase one last time after I fix the failing build.

agubler commented 5 years ago

@mwistrand perfect!!

agubler commented 5 years ago

@mwistrand Let me know when you've done the sanity check 😄