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.

Sign up for GitHub

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

Jump to bottom

Refactor code, drop more legacy stuff #158

Merged
merged 11 commits into from
Jan 16, 2021
Merged

Refactor code, drop more legacy stuff #158

merged 11 commits into from
Jan 16, 2021

Conversation

non25
Copy link
Contributor

@non25 non25 commented Jan 16, 2021 •
edited
Loading

Here's a branch for refactoring. I'll put some changes that I find reasonable here.
Get involved and tell me what you would want to see here.
Fixes #95, Closes #142, Closes #93, Closes #77

@non25 non25 changed the title Remove deprecation warnings and tests for them (tests fail) Refactor code, drop more legacy stuff Jan 16, 2021
benmccann
benmccann reviewed Jan 16, 2021
View reviewed changes
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR looks like a good idea to me

@non25

This comment has been minimized.

Sign in to view
@non25 non25 marked this pull request as ready for review January 16, 2021 08:14
@benmccann
Copy link
Member

This PR's a bit difficult to review since #159 was integrated. I just merged that one so if you rebase against master hopefully the diff for it won't appear here anymore

@non25
Copy link
Contributor Author

non25 commented Jan 16, 2021

This PR's a bit difficult to review since #159 was integrated. I just merged that one so if you rebase against master hopefully the diff for it won't appear here anymore

I've got two conflicts rebasing! OMG!
Done.

benmccann
benmccann reviewed Jan 16, 2021
View reviewed changes
README.md Outdated
},
extensions: ['.mjs', '.js', '.svelte'],
mainFields: ['svelte', 'browser', 'module', 'main']
...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was easier to paste into your webpack config when indented since it'd be indented there. I'd probably revert the indentation changes here and below

benmccann
benmccann reviewed Jan 16, 2021
View reviewed changes
README.md
@@ -231,30 +255,6 @@ module.exports = {
}
```

#### External Dependencies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this section removed?

Copy link
Contributor Author

@non25 non25 Jan 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because externalDependencies option was removed ?
or this option is purely for webpack ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right you are 😄

benmccann
benmccann reviewed Jan 16, 2021
View reviewed changes
@non25 @benmccann
Update README.md
fb34982 
Co-authored-by: Ben McCann <[email protected]>
@non25
Copy link
Contributor Author

non25 commented Jan 16, 2021 •
edited
Loading

Also I don't know about this line:

  • Add Node 14 support and fix intermittent crashes when using cache-loader in front of svelte-loader (#125)

There's no virtual stuff in repo anymore. Working cache was achieved by dropping virtual modules altogether.
Also #131 was fixed.

@benmccann benmccann merged commit 0c900f7 into sveltejs:master Jan 16, 2021
@non25 non25 deleted the refactor branch January 16, 2021 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmccann benmccann benmccann left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@non25 @benmccann @nergmada