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] Update rollup-plugin-typescript to rollup-plugin-typescript2 #568

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

kaffarell
Copy link
Collaborator

Will fix #560

@coveralls
Copy link

coveralls commented Oct 14, 2020

Coverage Status

Coverage increased (+1.3%) to 78.082% when pulling e9373d2 on update-typescript-plugin into 97e33f2 on master.

Bumps [tar](https://github.com/npm/node-tar) from 4.4.1 to 4.4.13. **This update includes security fixes.**
- [Release notes](https://github.com/npm/node-tar/releases)
- [Changelog](https://github.com/npm/node-tar/blob/master/CHANGELOG.md)
- [Commits](isaacs/node-tar@v4.4.1...v4.4.13)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Bumps [rollup](https://github.com/rollup/rollup) from 2.29.0 to 2.30.0.
- [Release notes](https://github.com/rollup/rollup/releases)
- [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md)
- [Commits](rollup/rollup@v2.29.0...v2.30.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Bumps [rollup](https://github.com/rollup/rollup) from 2.30.0 to 2.31.0.
- [Release notes](https://github.com/rollup/rollup/releases)
- [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md)
- [Commits](rollup/rollup@v2.30.0...v2.31.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
@atodorov
Copy link
Collaborator

atodorov commented Oct 16, 2020

Will fix #560

Can you add this to commit log and also rebase b/c the latest master branch updated a bunch of other dependencies.

Otherwise LGTM so not sure why the subject says "WIP".

@kaffarell
Copy link
Collaborator Author

kaffarell commented Oct 16, 2020

Ok, have done the changes. I included WIP in the title because there are a lot of other error down the line. I removed the IE fix but I cannot check if it is used because IE11 is still not working ( #445 ). The problems are typescript errors, every usage of options is using the configuration type but it also has other types.

* Public sortable object
* @param {Array|NodeList} sortableElements
* @param {object|string} options|method
*/
export default function sortable (sortableElements, options: configuration|object|string|undefined): sortable {
// get method string to see if a method is called
const method = String(options)
options = options || {}

So I could add a <configuration> before every usage of options but I don't think this is the best solution. I haven't found out when the object string or undefined type on options is used, so we could maybe remove them and write:
options: configuration

@kaffarell
Copy link
Collaborator Author

All the errors look like this:

semantic error TS2339: Property 'items' does not exist on type 'string | void | configuration | HTMLElement'.
  Property 'items' does not exist on type 'string'.

@lukasoppermann
Copy link
Owner

Hey @kaffarell please verify what I am saying, but I think you could cast options to here:

options = Object.assign({}, defaultConfiguration, store(sortableElement).config, options)
because at this point it will always be a configuration, correct?

Otherwise we would probably need to add a function that deals with the options parameter and returns a configuration, but try the above mentioned first please.

@lukasoppermann
Copy link
Owner

@kaffarell any news so this can be made mergable?

@lukasoppermann lukasoppermann self-assigned this Aug 30, 2022
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.

The dependecy 'rollup-plugin-typescript' is deprecated
4 participants