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

Relax validation upon outputPath and publicPath #78

Closed
davidmpaz opened this issue Jul 3, 2017 · 10 comments
Closed

Relax validation upon outputPath and publicPath #78

davidmpaz opened this issue Jul 3, 2017 · 10 comments

Comments

@davidmpaz
Copy link
Contributor

In getContentBase(webpackConfig) some validation is done against outputPath and publicPath.

I think this validation is checking too much and restricting use cases possible. Could we relax this to be a warning and not error?

My use case is:
Project with custom publishing strategy. Webpack is compiling into intermediate path which is then "published" to real publicPath. As consequence I am facing the error thrown in line 52

I think the validation should not be affected by outputPath since this can (or shall we allow it?) be anything.

I have found some other issue #59 and its PR #66 that might be related to same problem.

Does all this makes sense?

@davidmpaz
Copy link
Contributor Author

As an update here I can say that by commenting the line throwing the error I can get my webpack-dev-server working normally.

It is possible to get a warning or some message instead stopping execution?

thanks in advance

@weaverryan
Copy link
Member

@davidmpaz I'm certainly open to this - Encore purposely is "restrictive" now, to see if there are any legit use-cases.

Could you post the important parts of your webpack.config.js file and tell me a bit more about what exactly you're doing with respect to the intermediate path? And is the only issue when using the dev-server, or always?

@davidmpaz
Copy link
Contributor Author

davidmpaz commented Jul 6, 2017

Hi @weaverryan

thanks for answering. I can post relevant part but it is not more than that explained. See:

let publicPath = '/static/theme.dir'
Encore
    .setOutputPath('./static')
    .setPublicPath(publicPath)
    .setManifestKeyPrefix(publicPath)

the scenario is a framework that have many modules, themes are also modules. Every module can export their own static assets to an static directory inside module. Build/Deploy process take cares of putting those assets from every module in public directory accessible by application server, under webRooot/static/[module.name]. That's basically all.

I am affected only when trying to use dev-server. If I try to satisfy validation for making dev-server work, then I have a side effect under the manifest plugin, in manifest all urls are wrong.

Currently the workaround is to modify configuration after encore generation. I need to modify dev-server and manifest plugin settings as well to make all work like expected. This is ok for me, but when trying to introduce a whole development stack to colleagues this could be a draw back, it is always good when things looks natural and not hacky :)

// workaround method, I pass all path I need to override here
overrideGeneratedEncorePaths: function (
        webpackConfig, publicPath, publicPathSuffix, contentBase
    ) {
        if (typeof webpackConfig.devServer !== 'undefined') {
            webpackConfig.devServer.contentBase = contentBase;
            webpackConfig.devServer.publicPath =
                publicPath + '/' + publicPathSuffix + '/';
        }

        // update ManifestPlugin paths
        const ManifestPlugin = require('./node_modules/@symfony/webpack-encore/lib/webpack/webpack-manifest-plugin');

        let manifest = webpackConfig.plugins.find(function (current) {
            return current instanceof ManifestPlugin;
        });

        manifest.opts.basePath = manifest.opts.basePath + publicPathSuffix + '/';
        manifest.opts.publicPath = manifest.opts.publicPath + publicPathSuffix + '/';

        return webpackConfig;
    }

It is good to have Encore restrictive for ensuring good practices. I posted the issue because I think in this case restriction here is a limitation. It would be really nice if people developing with other frameworks could share experiences on this.

For my specific case. A warning stating that care must be taken setting urls when developing would be as good as throwing the error. Making this validation configurable by a flag or so, also would be good too maybe. Not sure though.

For building the whole project to deploy to production I don't have any issue since everything is working nice and dandy. Issue is all about improving developing experience.

regards

@weaverryan
Copy link
Member

👍 this makes sense - you have an extra "build" step effectively that moves the assets one last time after Encore is done. I'm going to first finish #66 with a twist (#66 (comment)) and then see what needs to still be done for this. #66 isn't quite the same issue - in that case they want the publicPath to truly be unaffected when you use the dev-server (i.e. to not be changed to the absolute URL to the dev server). But in your case, you do want that - just without the validation error :).

@davidmpaz
Copy link
Contributor Author

Great!! sounds perfect. Looking forward to check it out.

thanks!

@weaverryan
Copy link
Member

I haven't looked at this yet (but will after #96), but I think there is one tricky issue. We require the publicPath to be a substring of the outputPath, because we use this to determine the content base: https://github.com/symfony/webpack-encore/blob/v0.9.1/lib/config/path-util.js#L27

If we allow you to not intersect these - for a case like your's - how can Encore know what the contentBase (document root) should be for the dev-server? @davidmpaz you got this working with a complex hack around Encore - how was the contentBase determined?

Thx :)

@davidmpaz
Copy link
Contributor Author

I think that here:

We require the publicPath to be a substring of the outputPath

it is been required too much, it doesn't need to be the same. That's why I think this limitation is counterproductive.

for a case like your's - how can Encore know what the contentBase (document root) should be for the dev-server?

I think Encore doesn't need to know it. I am ensuring that all assets are published in document root (/) where dev-server is taking them from by appending the publicPath I gave. Currently, for example I am symlinking generated content in outputPath to my publicPath in document root, so dev-server is not affected at all.

you got this working with a complex hack around Encore - how was the contentBase determined?

the contentBase for the workaround was passed as variable, resolved by using path.

I am the opinion that contentBase should be determined from publicPath or manifestKeyPrefix without using outputPath because both can be intercepted but not necessarily.

Maybe is just a coincidence that it works only in my case when I remove the line throwing the error, I will try to debug what is happening in getContentBase(), once I get to office again, currently writing form home.

I will report back then

@weaverryan
Copy link
Member

Hey @davidmpaz!

Here's the tl;dr of the situation. The devServer.contentBase config is not needed. When you configure it, you see this message when you run dev-server:

Content not from webpack is served from /Users/weaverryan/Sites/os/symfony-demo/web

Without contentBase, only webpack assets can be accessed from the dev-server host. WITH contentBase, you can also reference other static assets that live in your document root (e.g. you could go to http://localhost:8080/favicon.ico to fetch the web/favicon.ico file in a Symfony project).

So, contentBase is a "nice to have", but not really needed. I think we should remove the Error and replace it with a warning:

Encore could not determine your document root. This is probably fine: the dev-server will still serve all of your webpack assets (but it won't serve any non-webpack assets).

@davidmpaz
Copy link
Contributor Author

@weaverryan thanks for the clarification. Sounds good to me to have the warning! clear message and not interrupting execution.

Not serving non-webpack assets through dev-server is not a problem at all, public folder is still served by built-in server (apache, nginx or php built-in).

thanks again for coming back to this.

@weaverryan
Copy link
Member

Closing - issue is old. Please open a new issue if needed! Thanks!

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

No branches or pull requests

2 participants