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

Iterator #65

Merged
merged 20 commits into from
Oct 3, 2021
Merged

Iterator #65

merged 20 commits into from
Oct 3, 2021

Conversation

nleanba
Copy link
Contributor

@nleanba nleanba commented Sep 10, 2021

Using the new async Iterator SynoGroup Lib

retog
retog previously requested changes Sep 11, 2021
Copy link
Contributor

@retog retog left a comment

Choose a reason for hiding this comment

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

I think the code is working because it relies on two different versions of '@factsmission/synogroup'. An older version which exports SynonymGroup as named export which is used by typescript for type-checks and a new version which is used at runtime. As the import resolved by the browser is js without types and webpack probably acts after typescript anyway, this cannot be used for types.

I think we have the following option:

  • re-create the node version of syno-group. As the es module cannot be used by webpack we would still be using the hack with the global variable and the external in webpack. Furthermore, this probably cannot be achieved nicely in the synogroup code, i.e. without duplicated and generated content as well as redundant configurations for npm/deno/tsc as this was the case in https://github.com/factsmission/synogroup/tree/fa0a58a0e501d07e117e3df2bba20d3321bf4c40.
  • Ignore the types (use any type or use js)
  • Use https://vno.land/ - I don't see any disadvantage in this approach. Sure it takes a couple of hours to migrate, but compare this with the time spent struggling with webpack, npm and babel issues.

src/components/JustificationView.vue Outdated Show resolved Hide resolved
src/globals.d.ts Outdated Show resolved Hide resolved
src/views/SynonymGrouper.vue Outdated Show resolved Hide resolved
src/views/SynonymGrouper.vue Outdated Show resolved Hide resolved
src/views/SynonymGrouper.vue Outdated Show resolved Hide resolved
@nleanba
Copy link
Contributor Author

nleanba commented Sep 11, 2021

I think the code is working because it relies on two different versions of '@factsmission/synogroup'. An older version which exports SynonymGroup as named export which is used by typescript for type-checks and a new version which is used at runtime. As the import resolved by the browser is js without types and webpack probably acts after typescript anyway, this cannot be used for types.

There's absolutely no requirement for the two library versions to be out of sync - but as you've stopped publishing npm packages, which seemed to be the easiest way to consume the typings, and as there have not been any breaking changes - the current configuration works.

I think we have the following option:

This should be relatively easy to do, and it's unclear to me what you mean by duplicated content?

At that point in time we had three generated code files: index.js, made by deno; index.d.ts, made by tsc; and a map for the latter, which probably was pointless and is easily removed from the tsc config. The two files neccesary for this aproach have very little (unnecessary) overlap.

  • Ignore the types (use any type or use js)

This would make the use of TypeScript pointless & I don't see any reasons to revert to untyped JS.

  • Use https://vno.land/ - I don't see any disadvantage in this approach. Sure it takes a couple of hours to migrate, but compare this with the time spent struggling with webpack, npm and babel issues.

I'd prefer to discuss and handle a potential move to vno at a later point and not mix it with functionality updates. Currently, our focus should be on replacing the classic view with what is now (still) the beta which would open many opportunities for optimization, as now there are a couple of shared components which thus require additional complexity or "adapters" to handle both pages.

This leaves the first option.

I would thus suggest the following next steps:

  1. consolidating (your) changes in SynoGroup into a final npm release (maybe even types only?)
  2. Implementing the requested/required code changes
    → here's where this PR would get merged
  3. Getting the Beta up to speed (See Issue #61)
  4. Remove the older classic view
  5. Remove unneccesary complexity in previously shared components
  6. ... then discuss / make changes in the general architecture, such as moving to JS and/or vno.
    I believe that these changes should be as easy, maybe even easier due to the achieved uniformity (compared to the current mix of mostly vue components, some JS utilities, JS "components", and TS utilities → by then, this should be reduced to vue components and TS utilities), to implement then as they are now.

@retog
Copy link
Contributor

retog commented Sep 12, 2021

I think the code is working because it relies on two different versions of '@factsmission/synogroup'. An older version which exports SynonymGroup as named export which is used by typescript for type-checks and a new version which is used at runtime. As the import resolved by the browser is js without types and webpack probably acts after typescript anyway, this cannot be used for types.

There's absolutely no requirement for the two library versions to be out of sync - but as you've stopped publishing npm packages, which seemed to be the easiest way to consume the typings, and as there have not been any breaking changes - the current configuration works.

There was never documented code to generate type declarations from the code, there was just a directory with some generated code in git, which is not as we do things.

I think we have the following option:

This should be relatively easy to do, and it's unclear to me what you mean by duplicated content?

all that is in https://github.com/factsmission/synogroup/tree/fa0a58a0e501d07e117e3df2bba20d3321bf4c40/package seems to b redundat with the content of the superfolder

At that point in time we had three generated code files: index.js, made by deno; index.d.ts, made by tsc; and a map for the latter, which probably was pointless and is easily removed from the tsc config. The two files neccesary for this aproach have very little (unnecessary) overlap.

type typing is already in the s file, and then there's that sourcemap.

  • Ignore the types (use any type or use js)

This would make the use of TypeScript pointless & I don't see any reasons to revert to untyped JS.

  • Use https://vno.land/ - I don't see any disadvantage in this approach. Sure it takes a couple of hours to migrate, but compare this with the time spent struggling with webpack, npm and babel issues.

I'd prefer to discuss and handle a potential move to vno at a later point and not mix it with functionality updates. Currently, our focus should be on replacing the classic view with what is now (still) the beta which would open many opportunities for optimization, as now there are a couple of shared components which thus require additional complexity or "adapters" to handle both pages.
The things is, the functionally isn't there. We weren't able to get the module from npmjs.org. The approach presented in this PR is a hack, even if the rutime and the buildtime library distribution are synced to the same version.

This leaves the first option.

The move to vno 9take maybe an hour or two, compare this with all the frustrated commit messages from getting things to work with webpack

I would thus suggest the following next steps:

1. consolidating (your) changes in SynoGroup into a final npm release (maybe even types only?)

2. Implementing the requested/required code changes
   → here's where this PR would get merged

3. Getting the Beta up to speed (See Issue [#61](https://github.com/factsmission/synospecies/issues/61))

4. Remove the older classic view

5. Remove unneccesary complexity in previously shared components

6. ... then discuss / make changes in the general architecture, such as moving to JS and/or vno.
   I believe that these changes should be as easy, maybe even easier due to the achieved uniformity (compared to the current mix of mostly vue components, some JS utilities, JS "components", and TS utilities → by then, this should be reduced to vue components and TS utilities), to implement then as they are now.

we have a working library, we have a working example only this consumer with its vue/npm/webpack/babel dependencies seems to have problems. Nobody suggested moving to JS, just if your platform doesn't work correctly with types then not using them is an approach to get things working quickly without the hack of using both an es-module distributed via HTTP and an npm package.

@retog
Copy link
Contributor

retog commented Sep 13, 2021

Creating a mode module (separate .d.ts files etc.) from a deno project seems to be a hard thing to do: denoland/deno#4528

@nleanba nleanba dismissed retog’s stale review October 3, 2021 10:07

Implemented requested changes

@nleanba nleanba merged commit 4642bfc into master Oct 3, 2021
@nleanba nleanba deleted the iterator branch October 3, 2021 10:23
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.

2 participants