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 BrowserSync support #2

Open
weaverryan opened this issue Jun 12, 2017 · 6 comments
Open

Add BrowserSync support #2

weaverryan opened this issue Jun 12, 2017 · 6 comments
Labels
Feature New Feature

Comments

@weaverryan
Copy link
Member

No description provided.

@weaverryan weaverryan added the Feature New Feature label Jul 2, 2017
@davidmpaz
Copy link
Contributor

Hi,

I would like to comment here since I have an initial solution but dont know if I am missing any scenario, any help would be appreciated ;)

Currently I have this set up :

const BrowserSyncPlugin = require('browser-sync-webpack-plugin');
config.plugins.push(new BrowserSyncPlugin(
    {
        proxy: serverUrl, // coming from some settings variables
        port: serverParts.port,
        files: [ // watching on changes
            {
                match: [
                    templatesPath + '/**/*.twig', // here can go php components as well
                    outputPath + '/**/*'
                ],
                fn: function (event, file) {
                    if (event === 'change') {
                        // get the named instance
                        const bs = require('browser-sync').get('bs-webpack-plugin');
                        bs.reload();
                    }
                }
            }
        ]
    },
    {
        reload: false, // this allow webpack server to take care of instead browser sync
        name: 'bs-webpack-plugin' // notice the name when getting instance above
    }
));

This will start webpack server on localhost:8080, so I can access any assets by requesting http://localhost:8080/static/js/app.js for example, the same for css files.

Php backend application is running in another port and browser-sync is proxying every request sent to it to this backend application. Everything nice and dandy so far.

Is this what we look after for this issue ?

Points noticed:

  1. If this plugin is added. we need to check dependency is added using loader-features.js. This module name is getting name from loaders, we are not adding a loader here but a plugin. Maybe some more general module is needed for checking features?
  2. Maybe for plugins we should do the same as for loaders, from point of view of module split (utilities). This involve some refactor also. So we can have a plugin dir with utils for adding plugins.

I haven't created PR basically because of point 1. I would like to check for dependencies but it doesn't look right just add that logic in loader-features.js module as another feature for browsersync for example.

Any other ideas ?

@weaverryan
Copy link
Member Author

Awesome! About (1), you're right - we'll need to expand loader-features to support plugins... in some way :). I'm not sure exactly how that will look. But, if you do want to open a PR with any solution (even if it's hacked together), that'll help push forward discussion about the best solution. I'm actually pretty excited to be able to have this :).

@davidmpaz
Copy link
Contributor

Happy to help ;), I am really interested to get best experience with the library :) ... I will need some time though, this week and next are packt for me, will see if find some free time on weekend to make a basic prototype to start discussion around.

cheers

weaverryan added a commit that referenced this issue Jul 25, 2017
This PR was squashed before being merged into the master branch (closes #99).

Discussion
----------

Refactor plugin configuration

Extract encore built in plugin configuration logic into plugin utils.

First step toward allowing to be able to configure plugins through options and have more modular integration for plugins inside encore. From now on only public API with proper options is needed,
for most cases.

This changes are internals and tries to improve plugin architecture inside encore. Please take a look on it and feedback are welcome :). So far I see improvements for several issues:

* #2 - Point .2 ...for plugins we should do the same as for loaders, from point of view of module split (utilities).
* #63 - Better (more modular) integration of the plugin as well its configuration.
* #65 - Same as #63
* #79 - Now it should be possible to pass options and/or check for flags to apply plugin or not.
* #87 - Some small steps to make clean plugin more configurable. Public API still need to be changed though.

thanks in advance.

Commits
-------

65adfdc Refactor plugin configuration
weaverryan added a commit that referenced this issue Aug 5, 2017
This PR was squashed before being merged into the master branch (closes #101).

Discussion
----------

Forked typescript type checking

This PR fixes: #63 it should be possible to merge also with #99 directly.

Open points:

Regarding point .1 in #2

> If this plugin is added. we need to check dependency is added using loader-features.js. This module name is getting name from loaders, we are not adding a loader here but a plugin. Maybe some more general module is needed for checking features?

After implementing this I realize that as  soon as you require a module which doesn't exist node throws an error, we don't even go through error catch from encore. So possible solutions I see for this pending point are:

1. We don't refactor `loader-features.js` file and we still will get a loud error when trying to use some plugin not installed. From error is clear whats need to be done.
2. We refactor `loader-features.js` to be named more general and we include plugins dependencies also there as intended initially. But this way we need to check for missing packages earlier in `WebpackConfig.js` like I am doing now in this PR.

I like either solutions the same, I would like to get feedback on what do you guys think about it?

thanks in advance.

Commits
-------

7a7e399 Forked typescript type checking
@davidmpaz davidmpaz mentioned this issue Aug 23, 2017
@InfopactMLoos
Copy link

I can't really say from the related commits but is this feature implemented or only in part? If it is maybe close the issue with that statement to clarify for others stumbling upon it?

@davidmpaz
Copy link
Contributor

The reason I guess this haven't been touch for so long is this comment

As I mention at the beginning, this PR will fail because of the nsp check it already reported but not got too much attention I guess on the browsersync side.

Not sure what else to do from my side :(

@InfopactMLoos
Copy link

Ah I seem to have missed that, thanks for the quick response. Sad to see such a lack of response from browsersync. Looking at the amount of open issues there I assume they don't have the capacity to handle all issues.

weaverryan added a commit that referenced this issue Nov 10, 2018
This PR was merged into the master branch.

Discussion
----------

Improve `copyFiles` method examples #2

Like in #442 Fix also `to` option in examples - paths are relative to build dir, not root dir.

Commits
-------

4b2ceda clarifying comment
676afcb Improve `copyFiles` method examples #2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants