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

Webpack loader #186

Merged
merged 11 commits into from
Dec 14, 2017
1 change: 1 addition & 0 deletions loader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = require('./build/tools/webpack-loader').default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to publish this file, please add it to "files" in package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

130 changes: 130 additions & 0 deletions src/tools/webpack-loader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/* @flow */

import * as babel from 'babel-core';

function shouldRunLinaria(source: string) {
return (
(/import .+ from ['"]linaria['"]/g.test(source) ||
/require\(['"]linaria['"]\)/g.test(source)) &&
/css(\.named)?`/g.test(source)
);
}

function transpile(source: string, map: any, filename: string) {
const file = new babel.File(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the user doesn't have .babelrc file at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses normal .babelrc/package.json/.babelrc.js resulution, so if user don't have a babel config it will just assume that the code is standard ES2015 without any experimental features.

As for inlining config in babel-loader, if it's possible to get the options from it through webpack, I do that, otherwise, the loader will accept inlined babel config.

{
filename,
sourceMaps: true,
inputSourceMap: map,
},
new babel.Pipeline()
);

// `transformFromAst` is synchronous in Babel 6, but async in Babel 7 hence
// the `transformFromAstSync`.
return (babel.transformFromAstSync || babel.transformFromAst)(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to use the async babel.transformFromAst

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no async version of transformFromAst in Babel 6.

Copy link
Member

@satya164 satya164 Dec 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant always use babel.transformFromAst, sync or async. You were handling promises anyway.

file.parse(source),
source,
{
filename,
sourceMaps: true,
inputSourceMap: map,
presets: [require.resolve('../../babel.js')],
Copy link
Member

@satya164 satya164 Dec 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to pass options to our babel preset via the loader options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, shouldn't it be presets: [require.resolve('../../babel.js'), loaderOptions] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? There are other options for babel that you might be interested in. Limiting it only to presets doesn't sound good. What if you want to pass plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, if you want to pass options to linaria preset it would look like this:

{
  loader: 'linaria/loader',
  options: {
    presets: [['linaria/babel', { /* options */ }]]
  }
}

Copy link
Member

@satya164 satya164 Dec 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to allow passing arbitrary presets and plugins? It's linaria-loader. People can use babel-loader for other babel stuff. Otherwise it'll just be a half-baked babel-loader with linaria preset included by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe you don't want all of out plugins to be applied? Only preval-extract?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's the usecase, but it should be an item in the options object if it needs to be exposed instead of overriding all plugins/presets.

parserOpts: file.parserOpts,
babelrc: false,
}
);
}

function getLinariaParentModules(fs: any, module: any) {
const parentModules = [];

function findLinariaModules(reasons) {
reasons.forEach(reason => {
if (!reason.module.resource) {
return;
}

const source = fs.readFileSync(reason.module.resource).toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to do fs.readFileSync(reason.module.resource, 'utf8'); instead of .toString(), though not sure if webpack's memory fs supports it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It supports it, but is there any actual reason to change it?

if (shouldRunLinaria(source)) {
parentModules.push({ source, filename: reason.module.resource });
}

findLinariaModules(reason.module.reasons);
});
}

findLinariaModules(module.reasons);

return parentModules;
}

const builtLinariaModules = [];

function linariaLoader(source: string, inputMap: any, meta: any) {
// If the module has linaria styles, we build it and we're done here.
if (shouldRunLinaria(source)) {
const { code, map } = transpile(source, inputMap, this.resourcePath);
builtLinariaModules.push(this.resourcePath);
return {
source: code,
map,
meta,
};
}

// Otherwise, we check for parent modules, which use this one
// and if they have linaria styles, we build them.
const parentModuleToTranspile = getLinariaParentModules(
this.fs.fileSystem,
this._module
);

parentModuleToTranspile.forEach(item => {
// We only care about modules which was previously built.
if (builtLinariaModules.indexOf(item.filename) > -1) {
transpile(item.source, null, item.filename);
}
});

return {
source,
map: inputMap,
meta,
};
}

function makeLoaderAdapter(fn) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This abstraction seems unnecessary. Why not call the function directly instead of creating a higher order function to which you pass it as an argument? We don't seem to need it to reuse anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it done, before I found a perfect solution and back them the whole loader was async making it harder to integrate with webpack.

function loaderAdapter(...args: any[]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code can be hugely simplified by making the result always async:

const callback = this.async();

Promise.resolve()
  .then(() => fn.call(this, ...args))
  .then(
    ({ source, map, meta }) => callback(null, source, map, meta),
    error => callback(error)
  );

In this case, it's better to use the second argument of .then here for the error callback instead of using .catch so that if callback throws error, it doesn't get caught and passed to callback again. At least that doesn't seem like the intended use of the API.

let error;
let results;
try {
results = fn.call(this, ...args);
} catch (e) {
error = e;
}

if (results && results instanceof Promise) {
const callback = this.async();
results
.then(({ source, map, meta }) => {
callback(null, source, map, meta);
})
.catch(e => {
callback(e);
});
} else if (results && !error) {
this.callback(null, results.source, results.map, results.meta);
} else {
this.callback(error);
}
}

Object.defineProperty(loaderAdapter, 'name', {
value: fn.name || loaderAdapter.name,
});

return loaderAdapter;
}

export default makeLoaderAdapter(linariaLoader);
2 changes: 1 addition & 1 deletion website/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ module.exports = (env = { NODE_ENV: 'development' }) => ({
{
test: /\.js$/,
exclude: /node_modules/,
use: { loader: 'babel-loader' },
use: [{ loader: 'babel-loader' }, { loader: 'linaria/loader' }],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's enable cache for babel-loader here to make sure we always test against it

},
].concat(
env.NODE_ENV === 'production'
Expand Down