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

non25 · 2021-01-15T04:55:34Z

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.

update native support

… version)

Fix minor typo

benmccann · 2021-01-15T14:37:33Z

test/loader.spec.js

@@ -369,7 +369,7 @@ describe('loader', () => {
 					function(err, code, map) {
 						expect(err).not.to.exist;

-						expect(code).to.contain(require.resolve('../lib/hot-api.js').replace(/[/\\]/g, '/'));
+						expect(code).to.contain(`lib/hot-api.js`);


use single quotes instead of backticks (if we apply svelte's linting to this repo at some point in the future that will be enforced. doing it now will make it easier to possibly setup)

benmccann · 2021-01-15T14:43:30Z

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 · 2021-01-15T15:13:36Z

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 ?

index.js

benmccann · 2021-01-15T16:49:41Z

index.js

@@ -20,30 +20,6 @@ const pluginOptions = {
 	markup: true
 };

-function makeHot(id, code, hotOptions) {


I noticed that @rixo's branch does not have the below options specified in pluginOptions

externalDependencies: true, hotReload: true, hotOptions: true, shared: true,

I think shared does not exist and perhaps we don't want to enable the others by default?

Where did you get this from?

The svelte-hmr branch: https://github.com/rixo/svelte-loader/tree/svelte-hmr

I think we should only disable hotReload by default and remove shared. The other two doesn't do anything in true/false manner.

It looks like the only thing it's used for is to determine if it's a compiler option:

svelte-loader/index.js

Line 81 in 0f1c49b

if (!pluginOptions[option]) compileOptions[option] = options[option];

Since none of these are compiler options we should probably remove them all

I think it is better to avoid making changes that are out of the scope of this PR.
Code could be improved, but in the other PR.
This place is hard to spot new comments on. 😕

Just to clarify, when I said "all" I meant the four options I originally suggested removing: externalDependencies, hotReload, hotOptions, shared

There shouldn't be any reason to pass those to the compiler since none of those four are valid compiler options: https://svelte.dev/docs#svelte_compile

Just to clarify, when I said "all" I meant the four options I originally suggested removing: externalDependencies, hotReload, hotOptions, shared

Doing this breaks tests and compilation. These options are PREVENTED from going to the compiler due to the pluginOptions object and code above.

Oh, I see. Sorry I had that backwards. It's a bit confusingly written. In rollup-plugin-svelte we moved them to be under compilerOptions. We should probably do that here too, but that can be a separate change. Just removing shared and leaving the rest as-is would make sense

It is done.

Yeah, that made me scratch my head for several minutes too. 😄

benmccann · 2021-01-15T16:57:28Z

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 · 2021-01-16T04:07:56Z

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 · 2021-01-16T04:23:54Z

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 · 2021-01-16T04:54:15Z

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

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 · 2021-01-16T05:07:26Z

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 · 2021-01-16T05:16:23Z

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 · 2021-01-16T05:31:09Z

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

rixo and others added 30 commits July 29, 2019 01:37

remove hard dep on svelte

45f83ef

style: remove trailing space

23fa354

extract makeHot from index.js

eda88c6

add support for HMR with svelte 3

5b568c8

Merge remote-tracking branch 'upstream/master' into hmr

dcf722e

add support for HMR with svelte-native

9dd5eec

introduce cmp.$$.replace instead of capture_state & inject_state

66b3061

better name for $$.replace

87b4192

fix restore props

76ec3b8

style: autofix

c4d9f09

update native support

21e0276

Merge pull request sveltejs#3 from halfnelson/hmr-native-fix

733852f

update native support

remove global polution, now that it isn't needed anymore

b2c1d29

style: remove trailing space

81cfdd2

refactor: extract svelte resolution to its own module

189a246

resolve svelte from application entrypoint - fixes sveltejs#109

74636fb

add support for overriding svelte location by env variable

0089526

Merge branch 'fix-link'

e106329

refactor: move makeHot to resolveSvelte (because it depends on svelte…

7787618

… version)

Merge branch 'hmr' into hmr-next

46db2fd

filter removed props using compilation infos

3122272

fix hmr support for context

5fbd73b

better debug name for file with multiple extensions

75bc4a3

Merge branch 'hmr' into hmr-next

638aa78

fix target / anchor in onMount when rendered to different ones (native)

67e98a8

fix support for native, & TNS 6

9026759

backport fixes from rollup plugin

e69f405

mock $capture_state to support preserving local state

d6e4a1a

respect noPreserveState option

91cfef7

implement full reload on failure

0e5b209

rixo and others added 6 commits August 27, 2020 11:38

Merge pull request sveltejs#9 from rendall/patch-1

f88b4c6

Fix minor typo

document that dev must be true for HMR

93feeb4

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

8b7d9bc

Drop Svelte 2 from HMR

b5afe79

Fix HMR test

7e65739

Move posixify back, remove unnecessary svelte-resolve

db9ff99

This was referenced Jan 15, 2021

fix: handle path with dollar signs #155

Closed

Remove Svelte 2 HMR #153

Closed

benmccann reviewed Jan 15, 2021

View reviewed changes

Change backticks to single quotes

780c961

benmccann reviewed Jan 15, 2021

View reviewed changes

index.js Outdated Show resolved Hide resolved

benmccann reviewed Jan 15, 2021

View reviewed changes

non25 added 2 commits January 16, 2021 05:45

Use matching imports in index.js

931dcc9

Merge README.md from rixo/svelte-loader#svelte-hmr

1ff2a35

non25 force-pushed the merge-rixo-hmr branch from fa5774d to ca550ea Compare January 16, 2021 04:03

Small changes that regards enabling emitCss and hotReload

ca550ea

non25 added 2 commits January 16, 2021 06:50

Options: disable hotReload by default, remove shared

7925e76

Revert hotReload

0f1c49b

Options: remove externalDependencies

bfb3b47

benmccann merged commit 7b81c00 into sveltejs:master Jan 16, 2021

non25 deleted the merge-rixo-hmr branch January 16, 2021 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

non25 commented Jan 15, 2021

benmccann Jan 15, 2021

benmccann commented Jan 15, 2021

non25 commented Jan 15, 2021

benmccann Jan 15, 2021

non25 Jan 16, 2021

benmccann Jan 16, 2021

non25 Jan 16, 2021

benmccann Jan 16, 2021

non25 Jan 16, 2021 •

edited

Loading

benmccann Jan 16, 2021 •

edited

Loading

non25 Jan 16, 2021 •

edited

Loading

benmccann Jan 16, 2021

non25 Jan 16, 2021 •

edited

Loading

benmccann commented Jan 15, 2021

non25 commented Jan 16, 2021 •

edited

Loading

benmccann commented Jan 16, 2021

non25 commented Jan 16, 2021 •

edited

Loading

benmccann commented Jan 16, 2021

non25 commented Jan 16, 2021

non25 commented Jan 16, 2021

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

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

Conversation

non25 commented Jan 15, 2021

benmccann Jan 15, 2021

Choose a reason for hiding this comment

benmccann commented Jan 15, 2021

non25 commented Jan 15, 2021

benmccann Jan 15, 2021

Choose a reason for hiding this comment

non25 Jan 16, 2021

Choose a reason for hiding this comment

benmccann Jan 16, 2021

Choose a reason for hiding this comment

non25 Jan 16, 2021

Choose a reason for hiding this comment

benmccann Jan 16, 2021

Choose a reason for hiding this comment

non25 Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

benmccann Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

non25 Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

benmccann Jan 16, 2021

Choose a reason for hiding this comment

non25 Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

benmccann commented Jan 15, 2021

non25 commented Jan 16, 2021 • edited Loading

benmccann commented Jan 16, 2021

non25 commented Jan 16, 2021 • edited Loading

benmccann commented Jan 16, 2021

non25 commented Jan 16, 2021

non25 commented Jan 16, 2021

non25 Jan 16, 2021 •

edited

Loading

benmccann Jan 16, 2021 •

edited

Loading

non25 Jan 16, 2021 •

edited

Loading

non25 Jan 16, 2021 •

edited

Loading

non25 commented Jan 16, 2021 •

edited

Loading

non25 commented Jan 16, 2021 •

edited

Loading