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

add eslint, custom search & fix mem leak #448

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

lynchem
Copy link
Collaborator

@lynchem lynchem commented Oct 7, 2023

NOTE: I'm just opening a draft for discussion. Possibly quite a few things here which should be changed first - this was done quickly for our needs. I tried to keep all of the changes that others have made since I forked.

Changes:

  • Added eslint config as original package had nothing. Tried to stay as close to original style as possible, but it wasn't 100% consistent either. I got rid of the trailing commas and spaces inside { are now everywhere, not just imports.
  • Keep track of blaze views created so we can destroy them when deleting the view.
  • move utils to common code and added some for tokenising search terms
  • customSearch property added which lets you plug in different searching logic, e.g. Atlas. This search works as Opt-in
  • allows passing your own searchTerm for the customSearch. I find it quite limiting to use the built-in datatables search box and want a LOT more filters.

Todo:

  • Has hard coded values for token creation in the search - should be in the table options
  • I always hated Tabular being an opt-out search so the custom search is opt-in. Should they both follow the same logic? Easiest would be for customSearch to follow that so as to not force a migration on people.
  • Write documentation.
  • Examples of using Atlas.
  • Test it

I won't have much time next week but will try to look at any feedback and update by the following one (16/10)
@StorytellerCZ @harryadel

@guncebektas guncebektas mentioned this pull request Oct 8, 2023
@jankapunkt
Copy link
Member

Hey @lynchem thanks a lot! I am checking this out now. I'm willing to merge this as is, however I'd like to emphasize, that we decided for ease of use to use standard / standardx as linter. My idea is to merge this and change the linter etc. later on as there many other things to be added here as well (tests, ci etc.)

@jankapunkt
Copy link
Member

@ricaragao do you also use aldeed:tabular? if so, I'd like to ask you to join here for review :-)

@harryadel
Copy link
Member

First off thank you so much @lynchem for following up to the forums thread and bringing this PR to the light!

I'm quite content with the current changes albeit there're two things that we're ought to do:

  1. Can you include tests for the changes you made?
  2. Create mini PRs. For the sake of this PR, it's ok to be merged as is. But in order to be able to review things and understand its impact it's best to split things up instead of lumping them in one giant PR.

Looking forward to your response, thanks!

@lynchem
Copy link
Collaborator Author

lynchem commented Oct 10, 2023

Hey folks. thanks for the feedback.

we decided for ease of use to use standard / standardx as linter. My idea is to merge this and change the linter etc. later on as there many other things to be added here as well (tests, ci etc.)

Yeah, I know storyteller mentioned standardjs. I didn't switch it direct though as the PR would be a lot bigger if I did. Personally I'm not a fan of leaving out the ; but happy to conform 😆

Can you include tests for the changes you made?

I can definitely add some unit tests.

Create mini PRs. For the sake of this PR, it's ok to be merged as is. But in order to be able to review things and understand its impact it's best to split things up instead of lumping them in one giant PR.

I don't think it's giant at this stage. And that's actually quite difficult to do to apply each in isolation. I think if we spend more time on it it would be better to address the issues listed in the initial comment.

@jankapunkt
Copy link
Member

@lynchem @harryadel I fixed the merge conflict. I think the biggest contribution would be a few tests so we can catch regressions in the future. If there are some tests added I can setup the CI. However, this is not as trivial using Tinytest (even with mtest) as it is using Mocha.

If you are fine with Mocha (which I suggest we should use to benefit from chai and sinon, too) then I could transform the existing tests and push them to this PR. What do you think?

Copy link
Collaborator Author

@lynchem lynchem left a comment

Choose a reason for hiding this comment

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

lgtm

@lynchem
Copy link
Collaborator Author

lynchem commented Oct 17, 2023

If you are fine with Mocha (which I suggest we should use to benefit from chai and sinon, too) then I could transform the existing tests and push them to this PR. What do you think?

If that's easy for you to do @jankapunkt that'd be great 👍🏻

@guncebektas
Copy link
Member

I have reviewed the code and it seems LGTM.
I'm not sure if customSearch should be included in the main repo. If we will, it should be documented.

@jankapunkt I have added a test in other pr #450 which might be a starting point for CI

@ricaragao
Copy link
Member

Hi @jankapunkt , sorry I missed your message. I will review and test it too.

@harryadel
Copy link
Member

Please guys let's not let this good PR go to waste. @ricaragao Did you test things out? @jankapunkt What else do we need before merging in?

@jankapunkt
Copy link
Member

From my end I'm rather waiting on some feedback from others who tested this a bit in their setups. I can also create an RC release if that's easier for testing. Just let me know.

Once I got some response / green light I will update the tests to mocha/chai/sinon and move forward.

@harryadel
Copy link
Member

Issuing a new release candidate would be amazing, please do so. Either way for @ricaragao or anybody else who wants to test things this would be a lot easier then modifying the package locally.

@lynchem
Copy link
Collaborator Author

lynchem commented Nov 22, 2023

Hi folks. Apologies. I've let this slip off my radar a bit. I should probably fix the first two elements so as not to make the package too confusing. And if you can update the tests that'd be great Jan. Let me try get something together tonight/tomorrow

@lynchem
Copy link
Collaborator Author

lynchem commented Nov 23, 2023

I've done those, just need a final test and tidy up and I'll push this afternoon.

* prefix all search props with 'search'
* allow some defaults to be configured
* switch default search inclusion to same as regular search - which is true
@StorytellerCZ
Copy link
Member

@lynchem am I to take that we are ready to proceed here to the next stage (RC)?

@jankapunkt jankapunkt marked this pull request as ready for review November 23, 2023 15:57
@lynchem
Copy link
Collaborator Author

lynchem commented Nov 23, 2023

Ok, I've made those changes

  • I prefixed all search props with 'search' so they're more clearly related
  • allow some defaults to be configured in the table options
  • switch default search inclusion to same as regular search - which is true

am I to take that we are ready to proceed here to the next stage (RC)?

Yes Jan... If you can publish a version I can test it in our project

@jankapunkt
Copy link
Member

@StorytellerCZ let's create an extra branch for this from master before merging into maste

@jankapunkt jankapunkt changed the base branch from master to devel November 23, 2023 16:00
@jankapunkt
Copy link
Member

let me quickly fix the merge conflicts

@jankapunkt
Copy link
Member

Okay I just found we have a little issue here @lynchem @StorytellerCZ

The devel branch is 8 commits behind but also 3 commits ahead of master. I would usually favour to merge into devel and only into master with a stable release.

I would try to simply merge master back into devel and update this PR so we should be good. Any objections?

@lynchem
Copy link
Collaborator Author

lynchem commented Nov 23, 2023

Doh... if you want to rebase the MCP devel branch I could sync my devel branch and cherry pick the commits I made in master into that so we have a devel->devel PR ? Or is there a more sensible way ?

@ricaragao
Copy link
Member

Hi guys, I was out. Let me know what I can help.

@jankapunkt
Copy link
Member

@lynchem I can also for now merge this into another master-based branch so you all can test this on your setups and in the meantime we can figure out the differences that we need from devel.

I recently replaced rebase + cherry-pick with git checkout --patch <branch> </path/to/file> which is interactive and let's me select which husks should remain or dropped during the merge file by file. Just fyi

@lynchem
Copy link
Collaborator Author

lynchem commented Nov 24, 2023

So I had a look at what the 3 commits that devel was ahead of master on. They were actually the result of a PR I made ages ago that @StorytellerCZ merged a couple months back. It was one change in 2 commits plus the merge commit from Jan. I had actually incorporated this in my changes in master as this was based off our local copy. I've just merged devel into master on my repo so that it doesn't see it as missing anything. I could now reset my devel branch to be master and we could switch this PR to go devel->devel or if you have a better idea let me know (or feel free to do it if its straightforward).

@jankapunkt
Copy link
Member

Hey @lynchem thanks for investigating. My proposal would be to simply replicate this procedure here, too:

  • merging devel into master
  • rebase devel to point to master
  • switch pr to point to devel
  • merge into devel
  • Bonus: if there are no conflicts also merge fix: sanitize data for xss #450 into devel then
  • release 2.2.0-rc.0

@lynchem
Copy link
Collaborator Author

lynchem commented Nov 27, 2023

Ok, cool. So I think it's ok to merge now. Once merged I'll reset my fork to the MCP one and do any future stuff in the devel branch. Once published I'll give it a test with one of our apps 👍🏻

@jankapunkt
Copy link
Member

jankapunkt commented Nov 27, 2023

@lynchem great I will merge tonight and let you know. By the way I can also add you to this repo as collaborator if you want, makes things a bit easier in terms of collectively pushing to PR branches, assign for reviews etc.

@jankapunkt jankapunkt merged commit 0058f89 into Meteor-Community-Packages:devel Nov 28, 2023
@jankapunkt
Copy link
Member

https://github.com/Meteor-Community-Packages/meteor-tabular/releases/tag/v2.2.2-rc.0

@harryadel
Copy link
Member

Great work guys 👏
Thank you @lynchem @jankapunkt and everybody else who worked on this.

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.

6 participants