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

[WIP] V3 #12

Open
wants to merge 52 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Rich-Harris
Copy link
Member

commented Nov 4, 2018

the interesting new stuff is in https://github.com/sveltejs/svelte-upgrade/tree/v3/test/v3/samples


Some TODOs:

  • update bind:/class:/etc directives to use braces for argument (bind:foo='bar' -> bind:foo={bar})
  • throw an error when methods and data have conflicting names
  • existing imports in components are often dropped; haven't dug into when this is triggered. try e.g.: <script>import foo from 'foo'; export default { helpers: { foo } };</script>
  • code importing onprops from 'svelte' is generated, which is not exported by Svelte (could be a Svelte problem; not sure what the intention is here)
  • computed properties are still rewritten as argument-less function calls, rather than reactive declarations
  • top-level statements in <script> are not moved into the separate 'static' script tag; they are left in the default per-instance script

@Rich-Harris Rich-Harris referenced this pull request Nov 5, 2018

Merged

[WIP] Implement reactive assignments #1839

0 of 1 task complete
@Conduitry

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

Absolutely not an important complaint, but: Could this please not clobber the EOF newline if the input file has one?

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

commented Nov 10, 2018

Good point — done

@Conduitry

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

I'm experimenting with running a few components I've written through this, and I just realized two things. 1) I apparently wrote a component with a method and a data item of the same name (!), and 2) this does not handle them well.

Things could be renamed if they were only used internally, but if both are exported, this is impossible to resolve automatically, as both get squished into one namespace. Perhaps another kind of warning or error message, indicating that manual edits are required?

@Conduitry

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

This also does not update class: directives to use braces around their argument, if that is something we settled on.

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

Yeah, it's not quite up to date — it's not fixing directives per the latest in the RFC. I was waiting to see if anyone strongly disagreed about that first. Same for ref:foo --> bind:this={foo}

Just realised I have some commits locally as well, will push those now

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

It might throw an error now if you have conflicting methods/data, I can't remember if I did that already

@Conduitry

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

Just pulled and built and re-ran and it looks like no, it does not complain about conflicting data and methods yet.

@rob-balfre

This comment has been minimized.

Copy link

commented May 7, 2019

Any plans to continue work on this? Got a large v2 code base and wanted to avoid a large refactor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.