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 Encore.configureRuntimeEnvironment() method to the public API #115

Closed

Conversation

Lyrkan
Copy link
Collaborator

@Lyrkan Lyrkan commented Jul 26, 2017

This PR fixes #109 by adding an Encore.configureRuntimeEnvironment and an Encore.clearRuntimeEnvironment method to the public API.


Basically, until now if you tried requiring @symfony/webpack-encore without using ./node_modules/.bin/encore, it would throw an Are you trying to require index.js directly? error.

This prevented retrieving the Webpack configuration object easily, which can be needed in some cases (e.g. if you use karma-webpack).

With these changes, requiring index.js doesn't create a WebpackConfig instance anymore if the runtime environment isn't available, which removes the need of having a check at that time.

In order to deal with methods of the public API that need the WebpackConfig to be available, a Proxy of the API is exported instead of the real object. This proxy only allows calls to configureRuntimeEnvironment and clearRuntimeEnvironment if the WebpackConfig isn't initialized, or act as a passthrough if it is.

This also fixes #55 since it allows to add a try { ... } catch { ... } quite easily around all the methods of the API.

The Encore.clearRuntimeEnvironment() (that nullifies runtimeConfig and webpackConfig) was needed to write tests but since I doubt it'll really be useful outside of that case let me know if you see another way to do it.


Examples:

const Encore = require('@symfony/webpack-encore');
Encore
    .setOutputPath('build/')
    .setPublicPath('/')
    .addEntry('main', './src/index.ts');

If executed directly would result in the following error:

Error: Encore.setOutputPath() cannot be called yet because the runtime environment doesn't appear to be configured. Try calling Encore.configureRuntimeEnvironment() first.

Whereas the following would work:

const Encore = require('@symfony/webpack-encore');
Encore
    .configureRuntimeEnvironment('dev-server')
    .setOutputPath('build/')
    .setPublicPath('/')
    .addEntry('main', './src/index.ts');

It is also possible to pass the same options that are available using the usual CLI tool using the second argument of configureRuntimeEnvironment and camel-cased names:

const Encore = require('@symfony/webpack-encore');
Encore
    .configureRuntimeEnvironment('dev-server', {
        keepPublicPath: true,
        https: true,
    })
    .setOutputPath('build/')
    .setPublicPath('/')
    .addEntry('main', './src/index.ts');

@Lyrkan Lyrkan force-pushed the add-configure-runtime-environment branch from 25c6acf to ce6e7d3 Compare July 27, 2017 10:52
Lyrkan pushed a commit to Lyrkan/encore-typescript-karma that referenced this pull request Jul 31, 2017
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Nice work! Sorry for the delay, but this was on my mind :). I was suspect of the Proxy at first, but it works great: even console.log() on the exported object prints as clearly as before.

// if the webpackConfig object hasn't been initialized yet.
const publicApiProxy = new Proxy(publicApi, {
get: (target, prop) => {
if (typeof target[prop] === 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be possible, but what about the else of this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I missed that part, I added a return target[prop] just in case.

index.js Outdated
];

if (!webpackConfig && (safeMethods.indexOf(prop) === -1)) {
throw new Error(`Encore.${prop}() cannot be called yet because the runtime environment doesn't appear to be configured. Try calling Encore.configureRuntimeEnvironment() first.`);
Copy link
Member

Choose a reason for hiding this comment

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

Something like:

Encore.${prop}() cannot be called yet because the runtime environment doesn't appear to be configured. Make sure you're using the encore executable or call Encore.configureRuntimeEnvironment() first if you're purposely not calling Encore directly.

A little long, but I still want to make sure people aren't making a mistake first.

Choose a reason for hiding this comment

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

that message brought me here today, that was shown by phpstorm :)

index.js Outdated

if (!webpackConfig && (safeMethods.indexOf(prop) === -1)) {
throw new Error(`Encore.${prop}() cannot be called yet because the runtime environment doesn't appear to be configured. Try calling Encore.configureRuntimeEnvironment() first.`);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

no need for the else here, since you properly explode above :)

index.js Outdated
} else {
// Either a safe method has been called or the webpackConfig
// object is already available. In this case act as a passthrough.
return (...parameters) => {
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of the ...parameters here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's needed in order to pass all the parameters to the real method: const res = target[prop](...parameters);

// Real API method
const inner = (param1, param2) => {
    console.log(`param1 = ${param1}`);
    console.log(`param2 = ${param2}`);
};

// Method returned by the proxy
const wrapper = (...parameters) => {
    inner(...parameters);
};

wrapper(1, 2);

// Output:
// param1 = 1
// param2 = 2

test/index.js Outdated
@@ -44,7 +65,12 @@ describe('Public API', () => {

describe('addEntry', () => {

it('should not be callable before the runtime environment has been configured', () => {
expect(() => api.addEntry('entry', 'main.js')).to.throw();
});
Copy link
Member

Choose a reason for hiding this comment

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

I think adding this everywhere is probably overkill. I think we only need to test that an exception is thrown once by using some non-safe method, and of course that no exception is thrown when using both of the safe methods. I think that's enough :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly what I thought after copy/pasting that part for every method of the API.

I wanted a 2nd opinion before removing everything :)

index.js Outdated
* Initialize the runtime environment.
*
* It can be used to directly manipulate the Encore API without
* executing the "./node_module/.bin/encore" utility.
Copy link
Member

Choose a reason for hiding this comment

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

This can be used to configure the Encore runtime if you're using Encore
without executing the "./node_module/.bin/encore" utility (e.g. with karma-webpack).

runtimeConfig = parseRuntime(
Object.assign(
{},
require('yargs/yargs')([environment]).argv,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this. Is there a need to continue to read the command line options? The command line options could be totally different (since there is a different executable being run).

I see that parseRuntime() looks for argv._[0] for the environment/command. Is that what you're trying to supply here? If so, I think we could set that manually - e.g. options._ = [environment]. Or, we could re-work parse-runtime.js so that the argv._[0] is actually done in encore.js and parse-runtime looked like function(command, options, cwd) {.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added that part to make sure that the first parameter passed to parseRuntime had the same structure than the one usually supplied by yargs, it doesn't actually keep reading the CLI options since we provide a fake process.argv (see https://github.com/yargs/yargs/blob/master/docs/api.md#api).

I did options._ = [environment] at first but switched to calling the API after realizing that yargs adds other things to the argv array that may be used later by parseRuntime (e.g.: $0)

index.js Outdated
logger.verbose();
}

webpackConfig = new WebpackConfig(runtimeConfig);
Copy link
Member

Choose a reason for hiding this comment

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

Since this (and the logger lines) above are repeated at the top of the file, let's warp these in some function (i.e. some simple function initializeWebpackConfig() at the top of the file) that we call in both places...

@weaverryan
Copy link
Member

Merged! This is really, really great. Thanks @Lyrkan!

@weaverryan weaverryan closed this Aug 10, 2017
weaverryan added a commit that referenced this pull request Aug 10, 2017
…lic API (Lyrkan)

This PR was squashed before being merged into the master branch (closes #115).

Discussion
----------

Add Encore.configureRuntimeEnvironment() method to the public API

This PR fixes #109 by adding an `Encore.configureRuntimeEnvironment` and an `Encore.clearRuntimeEnvironment` method to the public API.

---

Basically, until now if you tried requiring `@symfony/webpack-encore` without using `./node_modules/.bin/encore`, it would throw an `Are you trying to require index.js directly?` error.

This prevented retrieving the Webpack configuration object easily, which can be needed in some cases (e.g. if you use [karma-webpack](https://github.com/webpack-contrib/karma-webpack#usage)).

With these changes, requiring `index.js` doesn't create a `WebpackConfig` instance anymore if the runtime environment isn't available, which removes the need of having a check at that time.

In order to deal with methods of the public API that need the `WebpackConfig` to be available, a [Proxy](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy) of the API is exported instead of the real object. This proxy only allows calls to `configureRuntimeEnvironment` and `clearRuntimeEnvironment` if the `WebpackConfig` isn't initialized, or act as a passthrough if it is.

This also fixes #55 since it allows to add a `try { ... }  catch { ... }` quite easily around all the methods of the API.

The `Encore.clearRuntimeEnvironment()` (that nullifies `runtimeConfig` and `webpackConfig`) was needed to write tests but since I doubt it'll really be useful outside of that case let me know if you see another way to do it.

---

**Examples:**

```js
const Encore = require('@symfony/webpack-encore');
Encore
    .setOutputPath('build/')
    .setPublicPath('/')
    .addEntry('main', './src/index.ts');
```
If executed directly would result in the following error:
```
Error: Encore.setOutputPath() cannot be called yet because the runtime environment doesn't appear to be configured. Try calling Encore.configureRuntimeEnvironment() first.
```

Whereas the following would work:

```js
const Encore = require('@symfony/webpack-encore');
Encore
    .configureRuntimeEnvironment('dev-server')
    .setOutputPath('build/')
    .setPublicPath('/')
    .addEntry('main', './src/index.ts');
```

It is also possible to pass the same options that are available using the usual CLI tool using the second argument of `configureRuntimeEnvironment` and camel-cased names:

```js
const Encore = require('@symfony/webpack-encore');
Encore
    .configureRuntimeEnvironment('dev-server', {
        keepPublicPath: true,
        https: true,
    })
    .setOutputPath('build/')
    .setPublicPath('/')
    .addEntry('main', './src/index.ts');
```

Commits
-------

f760e52 Remove some tests related to the public API proxy and improve some texts/comments
ce6e7d3 Add Encore.configureRuntimeEnvironment() and Encore.clearRuntimeEnvironment() methods
@Lyrkan Lyrkan deleted the add-configure-runtime-environment branch August 10, 2017 07:20
weaverryan added a commit that referenced this pull request Sep 14, 2017
This PR was squashed before being merged into the master branch (closes #152).

Discussion
----------

Add various methods to configure default plugins

This PR adds some methods to configure default plugins (closes #148, closes #87 and closes #15):

* `Encore.configureDefinePlugin(callback)`
* `Encore.configureExtractTextPlugin(callback)`
* `Encore.configureFriendlyErrorsPlugin(callback)`
* `Encore.configureLoaderOptionsPlugin(callback)`
* `Encore.configureUglifyJsPlugin(callback)`

Other changes:

* Allows the `clean-webpack-plugin` to be configured using `Encore.cleanupOutputBeforeBuild(paths, callback)`
* Fixed a small mistake in the `Encore.configureManifestPlugin()` jsdoc
* Reorganized flags/callbacks in the constructor of `webpack.config.js` since it was starting to be a bit hard to read. I'm not sure about the way I splitted things though, let me know what you think about it.
* Renamed `common-chunks.js` to `commons-chunks.js` since the name of the plugin is `CommonsChunkPlugin`

@weaverryan: Not directly related but while doing all of that I noticed that `sassOptions` uses snake-case whereas I used camel-case in #144 for `enablePreactPreset()` and in #115 for `configureRuntimeEnvironment()`, should we do something about that?

***Edit 1:** Added `Encore.configureCleanWebpackPlugin()`*
***Edit 2:** Removed `Encore.configureCleanWebpackPlugin()` to use `Encore.cleanupOutputBeforeBuild(paths, callback)` arguments instead*

Commits
-------

286787a Minor text changes
f72614b Remove configureCleanWebpackPlugin and use cleanupOutputBeforeBuild arguments instead
90c8565 Add various methods to configure default plugins
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Nov 16, 2018
…onment method (Lyrkan, weaverryan)

This PR was merged into the 3.4 branch.

Discussion
----------

[Encore] Add a section about the configureRuntimeEnvironment method

This PR adds some info about the `configureRuntimeEnvironment(...)` method of Encore.

I wasn't sure where to put that section, it's not an "advanced config" per-se, but it didn't feel right to add it to the FAQ either... but maybe that's just me?

**Feature PR:** symfony/webpack-encore#115
**Closes:** symfony/webpack-encore#233

Commits
-------

24b3370 minor tweak
653ccff [Encore] Add a section about the configureRuntimeEnvironment method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get config for testing configuration (Karma) Improve error display when thrown inside Webpack.config.js
3 participants