sveltejs / svelte-loader

Webpack loader for svelte components.
MIT License
597 stars 72 forks source link

Replace broken Svelte 2 HMR with the one from rixo #156

Closed non25 closed 3 years ago

non25 commented 3 years ago

I've moved some code around to drop Svelte 2 hmr support, removed unneccessary code to make diffs smaller and easier to understand. Tested on both webpack 4 and 5, it works. I think it is fair to look only for index.js, package.json and test/loader.spec.js changes.

benmccann commented 3 years ago

Are you pulling the code from https://github.com/rixo/svelte-loader/tree/svelte-hmr ? At the very least, this PR doesn't appear to have the README changes that are there

non25 commented 3 years ago

Are you pulling the code from https://github.com/rixo/svelte-loader/tree/svelte-hmr ? At the very least, this PR doesn't appear to have the README changes that are there

What do you mean by that? That we should pull changes to the readme? For me the only relevant part is:

            // NOTE Svelte's dev mode MUST be enabled for HMR to work
            // -- in a real config, you'd probably set it to false for prod build,
            //    based on a env variable or so
            dev: true,

            // NOTE emitCss: true is currently not supported with HMR
            emitCss: false,
            // Enable HMR
            hotReload: true, // Default: false
            // Extra HMR options
            hotOptions: {
              // Prevent preserving local component state
              noPreserveState: false,

              // If this string appears anywhere in your component's code, then local
              // state won't be preserved, even when noPreserveState is false
              noPreserveStateKey: '@!hmr',

              // Prevent doing a full reload on next HMR update after fatal error
              noReload: false,

              // Try to recover after runtime errors in component init
              optimistic: false,

              // --- Advanced ---

              // Prevent adding an HMR accept handler to components with 
              // accessors option to true, or to components with named exports
              // (from <script context="module">). This have the effect of
              // recreating the consumer of those components, instead of the
              // component themselves, on HMR updates. This might be needed to
              // reflect changes to accessors / named exports in the parents,
              // depending on how you use them.
              acceptAccessors: true,
              acceptNamedExports: true,
            }
          }
        }
      }

Can we discuss code first ?

benmccann commented 3 years ago

I was suggesting you replace the README with the version at https://github.com/rixo/svelte-loader/blob/svelte-hmr/README.md. I would just copy the whole thing over. The code generally looks fine to me

non25 commented 3 years ago

Are you pulling the code from https://github.com/rixo/svelte-loader/tree/svelte-hmr ? At the very least, this PR doesn't appear to have the README changes that are there

I was pulling code from https://github.com/rixo/svelte-loader-hot and was confident that you pointing at svelte-loader-hot too. The same info was in my first commit message

Merge branch 'master' of svelte-loader-hot into merge-rixo-hmr

That could explain my confusion on merging README.md part.

I did it that way because I know that svelte-loader-hot works as I'm using it daily.

benmccann commented 3 years ago

https://github.com/rixo/svelte-loader/tree/svelte-hmr is almost the same as your PR. I compared them. The only things to update are the options I mentioned and the README. If you update those then we can probably merge this

non25 commented 3 years ago

What did you compare, can you send me a link? I compared this: https://github.com/non25/svelte-loader/compare/merge-rixo-hmr...rixo:svelte-hmr Can't say that they are almost the same. :thinking:

It looks like hotReload in plugin options describes that this option is possible to pass at all. Not that hotReload is turned on by default.

I've just tested it by omitting hotReload from webpack.config.js. It is not enabled by default.

benmccann commented 3 years ago

You can't compare on the web interface for some reason. I'm not sure why it's screwed up. I had to pull down both repos locally and switch to the relevant branches to compare them.

non25 commented 3 years ago

Interesting, didn't know that. Just cloned both repos, compared and yes, they almost the same.

Deleting options breaks tests. I would leave them for now and concentrate on releasing working HMR and working Webpack 5.

We can refactor later.

non25 commented 3 years ago

I have finally understood what pluginOptions do. They prevent passing described options from options object to the compiler.