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

Clean up the public API file and improve a few other things #550

Closed
wants to merge 5 commits into from

Conversation

Lyrkan
Copy link
Collaborator

@Lyrkan Lyrkan commented Mar 31, 2019

Now that we don't have many active PRs anymore I figured it was a good time to do some spring cleaning, especially in the index.js file (since we often recommend users to look at its content).

This PR:

  • Moves the proxy creation into its own file (lib/EncoreProxy.js)

  • Moves the content of initializeWebpackConfig(...) inside of the WebpackConfig's constructor (we already had some checks related to the RuntimeConfig there anyway)

  • Fixes a few JSDoc issues (missing or misplaced comments, types not resolving correctly, etc.)

  • Puts examples from the index.js file's comments inside of Markdown code-blocks, which are supported by at least PHPStorm and VSCode:

    • In PHPStorm
    Before After
    image image
    Before After
    image image
  • Indicates that Encore.getWebpackConfig() returns a Webpack configuration, which allows IDEs' autocompletion to work on it:

    image

Kocal added a commit to Kocal/webpack-encore that referenced this pull request Apr 7, 2019
@@ -12,7 +12,7 @@

const parseRuntime = require('../lib/config/parse-runtime');
const context = require('../lib/context');
const chalk = require('chalk');
const chalk = require('chalk').default;
Copy link
Member

Choose a reason for hiding this comment

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

.default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah... it doesn't change anything at runtime since require('chalk') is the same thing than require('chalk').default but it seems that due to how chalk is typed, VSCode (which uses TypeScript to resolve things) doesn't find its properties if you don't specify the .default:

image

image

initializeWebpackConfig();
}
let runtimeConfig = context.runtimeConfig;
let webpackConfig = runtimeConfig ? new WebpackConfig(runtimeConfig, true) : null;
Copy link
Member

Choose a reason for hiding this comment

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

If there is no runtimeConfig, when is this variable initialized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In Encore.configureRuntimeEnvironment(...)

That was already the case before, I basically just replaced calls to initializeWebpackConfig(...) by new WebpackConfig(...) (since the whole "validation" part is now done by the constructor).

@@ -9,6 +9,7 @@

'use strict';

const WebpackConfig = require('./WebpackConfig'); //eslint-disable-line no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize we needed to import the class in order for the jsdoc to function below. If that's the case, I'm surprised eslint isn't smart enough to know that this really is "used"

Copy link
Member

Choose a reason for hiding this comment

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

well, everything is scoped in JS. As our class is not a global variable, you cannot use it globally.

@weaverryan
Copy link
Member

Thank you @Lyrkan!

@stof
Copy link
Member

stof commented Apr 12, 2019

@Lyrkan microsoft/TypeScript#14377 seems to suggest a way to reference a type from another module in jsdoc without importing the file at runtime. It might be worth trying it.

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Apr 12, 2019

Microsoft/TypeScript#14377 seems to suggest a way to reference a type from another module in jsdoc without importing the file at runtime. It might be worth trying it.

@stof Yep, in a first version I actually used:

/** @typedef {import('./WebpackConfig')} WebpackConfig */

It works fine in VSCode but since that's not valid JSDoc I went for require(...) instead.

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.

None yet

3 participants