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

Add `--target=lib` flag #42

Merged
merged 10 commits into from Jul 9, 2019

Conversation

@mwistrand
Copy link
Contributor

commented Sep 12, 2018

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.

@mwistrand mwistrand requested review from agubler and matt-gadd Sep 12, 2018

@agubler
Copy link
Member

left a comment

A few observations. Also how does this behave with being ejected?

src/lib.ts Outdated
postcssCustomProperties({ preserve: 'computed' })
]);

return globby(cssFiles).then(files => {

This comment has been minimized.

Copy link
@agubler

agubler Sep 13, 2018

Member

no await? Might be a bit clearer.

This comment has been minimized.

Copy link
@mwistrand

mwistrand Sep 13, 2018

Author Contributor

That would improve readability here.

src/lib.ts Outdated
}

const spinner = ora().start('Cleaning previous output');
return createTask((callback: any) => rimraf(outDir, callback))

This comment has been minimized.

Copy link
@agubler

agubler Sep 13, 2018

Member

All these could be awaited also perhaps?

This comment has been minimized.

Copy link
@mwistrand

mwistrand Sep 13, 2018

Author Contributor

I'll see what I can come up with, but personally I prefer the then series to a try/catch block in this particularly case.

src/lib.ts Outdated
postcssImport(),
postcssPresetEnv(postcssPresetConfig),
postcssModules({
generateScopedName: mode === 'dist' ? '[hash:base64:8]' : '[name]__[local]__[hash:base64:5]',

This comment has been minimized.

Copy link
@agubler

agubler Sep 13, 2018

Member

We have standardised on just '[name]__[local]__[hash:base64:5]' now for all modes for the other build commands

This comment has been minimized.

Copy link
@mwistrand

mwistrand Sep 13, 2018

Author Contributor

Thanks, I should have looked at what we were already using in the repo instead of what @dojo/widgets was using from @dojo/grunt-dojo2.

src/lib.ts Outdated
const outDir = `output/${mode}`;
const cssFiles = path.join(basePath, outDir, '**/*.css');
const packageJsonPath = path.join(basePath, 'package.json');
const packageJson = fs.existsSync(packageJsonPath) ? require(packageJsonPath) : {};

This comment has been minimized.

Copy link
@agubler

agubler Sep 13, 2018

Member

Should we support building the a widget lib without a package json?

This comment has been minimized.

Copy link
@mwistrand

mwistrand Sep 13, 2018

Author Contributor

Nope!

@agubler

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

@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

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

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

@mwistrand

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

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

@mwistrand mwistrand force-pushed the mwistrand:35-library-target branch from b35abc9 to fec5b12 Sep 14, 2018

@mwistrand

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

@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

@mwistrand mwistrand force-pushed the mwistrand:35-library-target branch from 12c3ed7 to c12c2d6 Feb 3, 2019

@codecov

This comment has been minimized.

Copy link

commented Feb 3, 2019

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%> (ø) ⬆️
src/util.ts 100% <100%> (ø) ⬆️
src/logger.ts 100% <100%> (ø) ⬆️
src/base.config.ts 24% <2.7%> (-9.83%) ⬇️
src/dist.config.ts 37.5% <27.27%> (-6.95%) ⬇️
src/dev.config.ts 33.33% <60%> (+2.56%) ⬆️
src/main.ts 95.31% <82.35%> (+0.03%) ⬆️

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

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

@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

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

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

@agubler

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

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 mwistrand force-pushed the mwistrand:35-library-target branch from c12c2d6 to cb1683a Mar 14, 2019

@mwistrand

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@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.

@mwistrand mwistrand force-pushed the mwistrand:35-library-target branch from cb1683a to 224864a Mar 15, 2019

@agubler

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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

@agubler

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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

@mwistrand mwistrand force-pushed the mwistrand:35-library-target branch from f112f91 to 936b7f4 Apr 10, 2019

src/main.ts Outdated
const basePath = process.cwd();
libEntryPath = path.join(basePath, 'src', 'main.ts');
if (!fs.existsSync(libEntryPath)) {
const tmpDir = fs.mkdtempSync(os.tmpdir());

This comment has been minimized.

Copy link
@willstott101

willstott101 May 7, 2019

Thanks for your work on this. It's going to be very useful!

Whilst trying to get this setup I've found this line doesn't work on linux as os.tmpdir does not include the path separator. This means the build tries to make a folder in the root of the filesystem.

util.ts in this repo already works around it with:

fs.mkdtempSync(`${os.tmpdir()}${path.sep}`)

I'm curious about on which OS this works for you?

This comment has been minimized.

Copy link
@mwistrand

mwistrand May 10, 2019

Author Contributor

Thanks for catching this. It's been working for me on macOS and Windows 10/11, but I'll update this as well.

@willstott101

This comment has been minimized.

Copy link

commented May 7, 2019

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.

  • This tool makes some css and JS files.
  • In an app I install my widget library built with this tool.
  • The app build at some point additionally mangles the css class names.
  • This second mangling step is not correctly honoured in the JS.
@agubler
Copy link
Member

left a comment

@mwistrand This is super close, just a few suggestions and a heads up that we are probably going to land #56 (changes the CE builds to using a single configuration rather than a configuration for each element), which will mean this will need a bit of a rebase (shouldn't be too much!).

Also have been thinking about the issue with creating a legacy and modern build together, for now to reduce the complexity I am going to reverse a bit and say that we should only support producing one at a time. It will be down to the consumer to run the command twice and package (copy, move etc) the output as needed for their final release bundled.

return entry;
}, {}),
mode: args.target === 'lib' ? 'none' : 'development',
entry:

This comment has been minimized.

Copy link
@agubler

agubler May 9, 2019

Member

Instead of creating a temporary file or using an existing main.ts as the entry point, we could iterate through the widgets and use the widget.path as the entry point?

For filtering the webpack assets we could prefix the js and css output name with something like throw-away, which would leave all the other assets.

This comment has been minimized.

Copy link
@agubler

agubler May 9, 2019

Member

Something like

elements.reduce((entry: any, element: any) => {
    entry[element.name] = [element.path];
    return entry;
}, {});
const outputPath = output!.path as string;
const modeOutputPath = path.join(outputPath, location);

if (module && args.target === 'lib') {

This comment has been minimized.

Copy link
@agubler

agubler May 9, 2019

Member

Can we do this in the base and change based on mode and target? It would save having to have a duplication to edit the ts-loader configuration.

In another change @rorticus is turning transpileOnly off for building custom elements, so that wouldn't need to be updated for the lib target.

This comment has been minimized.

Copy link
@agubler
config.plugins = removeEmpty([
...plugins!,
(args.target !== 'lib' || args.clean) && new CleanWebpackPlugin([location], { root: outputPath, verbose: false }),
args.target === 'lib' &&

This comment has been minimized.

Copy link
@agubler

agubler May 9, 2019

Member

Can we include the EmitAllPlugin in the base config as well and set inlineSourceMaps to false for both dev and dist outputs.

@@ -40,7 +40,8 @@ export default function logger(stats: any, configs: any[], runningMessage: strin

return child.assets.map((asset: any) => {
const size = (asset.size / 1000).toFixed(2);
return `${entry}/${asset.name} ${chalk.yellow(`(${size}kb)`)}`;
const assetName = isLibrary ? asset.name.replace(/^\//, '') : `${entry}/${asset.name}`;

This comment has been minimized.

Copy link
@agubler

agubler May 9, 2019

Member

How come we need to strip the leading / here?

src/main.ts Outdated

const basePath = process.cwd();
libEntryPath = path.join(basePath, 'src', 'main.ts');
if (!fs.existsSync(libEntryPath)) {

This comment has been minimized.

Copy link
@agubler

agubler May 9, 2019

Member

As per the comment above, if we use the element.path as the module to load we shouldn't need to create any temporary files.

@agubler

This comment has been minimized.

Copy link
Member

commented May 9, 2019

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

@mwistrand

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

@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

This comment has been minimized.

Copy link

commented May 12, 2019

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?

@mwistrand mwistrand force-pushed the mwistrand:35-library-target branch from 62c7a87 to 2f04e46 May 15, 2019

"chalk": "2.4.1",
"clean-webpack-plugin": "1.0.0",
"cli-columns": "3.1.2",
"copy-webpack-plugin": "^4.6.0",

This comment has been minimized.

Copy link
@agubler

agubler May 17, 2019

Member

@mwistrand Can we pin all the dependencies please?

@mwistrand mwistrand force-pushed the mwistrand:35-library-target branch from 3f0cae7 to ed0626a Jun 8, 2019

src/main.ts Outdated
describe: 'the type of project',
alias: 't',
default: 'widget',
choices: ['widget', 'lib']

This comment has been minimized.

Copy link
@matt-gadd

matt-gadd Jun 9, 2019

should we perhaps change widget to custom element now?

config.plugins = [...plugins!, new CleanWebpackPlugin([location], { root: outputPath, verbose: false })];
config.plugins = removeEmpty([
...plugins!,
(args.target !== 'lib' || args.clean) && new CleanWebpackPlugin([location], { root: outputPath, verbose: false })

This comment has been minimized.

Copy link
@matt-gadd

matt-gadd Jun 9, 2019

I can't see where this args.clean is set, now that we are building the dev and dist separately do we actually need this different behaviour anymore?

This comment has been minimized.

Copy link
@mwistrand

mwistrand Jun 21, 2019

Author Contributor

I believe it can be removed (from both here and the dist config), but I'll double-check before blindly doing so.

]);

if (args.target === 'lib') {
config.devtool = 'source-map';

This comment has been minimized.

Copy link
@matt-gadd

matt-gadd Jun 9, 2019

isn't this the default for production anyway? if so we can just set it for both and drop the if as it won't harm.

} as any),
args.target === 'lib' &&
new CopyWebpackPlugin(
['src/**/*.css.d.ts', assetGlob].map(from => ({

This comment has been minimized.

Copy link
@matt-gadd

matt-gadd Jun 9, 2019

won't any assets in the webpack build get copied over anyway?

@mwistrand mwistrand force-pushed the mwistrand:35-library-target branch 3 times, most recently from 09a9603 to 548c956 Jun 17, 2019

@agubler

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

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

@mwistrand

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

@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)).

mwistrand added 7 commits Apr 10, 2019
Add support for building widget libraries
Add a `--target=lib` flag that, when set, compiles and emits the
specified widgets as individual files within a library.

Add Cypress tests

Tests behave as follows:
- Build the lib
- Copy the output back into the test-app
- Build the test app
- Execute tests against that application
Updates
- Upgrade MiniCssExtractPlugin for asset fix
- Simplify config based on review feedback
Fix/update the ejected config
- Fix: the custom-element.js template should be copied to
`config/build-widget` on eject.
- Add `--env.target` to the ejected config
- Generate single configs for all elements instead of mapping each
element to a separate webpack config

@mwistrand mwistrand force-pushed the mwistrand:35-library-target branch from a7a8260 to 206ffd2 Jun 28, 2019

@agubler

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

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

@mwistrand

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@mwistrand perfect!!

@agubler

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

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

mwistrand added 2 commits Jul 1, 2019
Remove copy-webpack-plugin
As managing d.ts files for library builds will be handled by the
EmitAllPlugin, the copy-webpack-plugin and corresponding types packages
are no longer necessary as dependencies.
@agubler
agubler approved these changes Jul 9, 2019

@mwistrand mwistrand merged commit 29f4cdf into dojo:master Jul 9, 2019

3 of 5 checks passed

codecov/patch 44.94% of diff hit (target 70.67%)
Details
codecov/project 64.5% (-6.18%) compared to dc78b9e
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@mwistrand mwistrand deleted the mwistrand:35-library-target branch Aug 7, 2019

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