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

Reverting change to only package vue runtime loader #769

Merged
merged 3 commits into from
May 13, 2020

Conversation

weaverryan
Copy link
Member

Using the runtime build by default makes it harder for beginners and it's hard to know the fix. Specifically, if you follow the "intro docs" to Vue, you will likely try this:

var app = new Vue({
  el: '#app',
  data: {
    message: 'Hello Vue!'
  }
})

which will not work (and the error will be tough for beginners to spot). As a compromise, on a production build, we print a recommendation so that people can discover the option to use the smaller build.

@weaverryan weaverryan force-pushed the revert-vue-runtime-only branch 4 times, most recently from 591bb5d to 1991f0b Compare May 13, 2020 02:09
@Lyrkan
Copy link
Collaborator

Lyrkan commented May 13, 2020

Displaying the recommendation during prod builds only may be too late I think. If someone starts working on an app only using encore dev/encore dev-server they could use things that won't be compatible with the runtime-only build and then need to do a lot of refactoring in order to be able to use it once they learn about the alias. Also, having a CSP compliant build by default could prevent other issues that are hard to diagnose for beginners.

Couldn't we detect the error thrown by Vue and make it easier to understand instead (with an example of how to add the alias)?

@Lyrkan
Copy link
Collaborator

Lyrkan commented May 13, 2020

Also, the failing test looks like the one I had to fix recently when working on Webpack 5 : https://github.com/symfony/webpack-encore/pull/645/files#r422717443

@weaverryan
Copy link
Member Author

Couldn't we detect the error thrown by Vue and make it easier to understand instead (with an example of how to add the alias)?

I don't think so? You see the error at runtime. For example, in Vue 3 (this is the source .ts, but you get the general idea of how this looks in the final dist file), you get the error simply because the compile() function is set to one that throws an error: https://github.com/vuejs/vue-next/blob/e954ba21f04f0ef848c687233fcb849d75e4153f/packages/vue/src/runtime.ts#L8

I don't think you can detect that at build time, well, at least not easily.

We could move the "recommendation" to show on any build... I just thought it might be annoying... though being annoying is not the worst thing here. CSP is great, but it's not our main use-case as afaik, it's only a problem in environments like Chrome apps.

@stof
Copy link
Member

stof commented May 13, 2020

We could move the "recommendation" to show on any build... I just thought it might be annoying... though being annoying is not the worst thing here. CSP is great, but it's not our main use-case as afaik, it's only a problem in environments like Chrome apps.

Or any site where you deploy CSP.

lib/config-generator.js Outdated Show resolved Hide resolved
lib/WebpackConfig.js Outdated Show resolved Hide resolved
@weaverryan
Copy link
Member Author

Ok, compromise time! With help from Stof:

  1. This PR still defaults to the "full" build. This helps beginners.
  2. We now print a recommendation on ALL builds, AND that recommendation includes some info about CSP. So if CSP is not something you've thought about, we've just pinged you about it ;)

To create a smaller (and CSP-compliant) build, see https://symfony.com/doc/current/frontend/encore/vuejs.html#runtime-compiler-build

That points to a docs PR I just made: https://github.com/symfony/symfony-docs/pull/13664/files

  1. You can silence the warning (if you really DO want a runtime build and don't like seeing our nice blue message on each build) by explicitly setting runtimeCompilerBuild: true.

This makes it harder for beginners and it's hard to know the fix.
As a compromise, this alerts that there is
an option to choose a smaller build.
index.js Outdated Show resolved Hide resolved
Co-authored-by: Vincent Le Biannic <850046+Lyrkan@users.noreply.github.com>
@weaverryan weaverryan merged commit adbadc3 into symfony:master May 13, 2020
weaverryan added a commit to symfony/symfony-docs that referenced this pull request May 13, 2020
This PR was merged into the 5.0 branch.

Discussion
----------

[WCM] Adding info about Vue build

Docs for symfony/webpack-encore#769

Commits
-------

e7bff3f adding info about Vue build
@weaverryan weaverryan deleted the revert-vue-runtime-only branch May 18, 2020 00:39
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