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

Combine V4 & V5 together #2248

Merged
merged 15 commits into from
Jan 23, 2020
Merged

Combine V4 & V5 together #2248

merged 15 commits into from
Jan 23, 2020

Conversation

danielkcz
Copy link
Contributor

@danielkcz danielkcz commented Dec 31, 2019

We talked about in #2120.

I created src/v4 and copied mobx4-master over to it. The src/v5 is source of a current master. That was an easy part :) In some distant future, we might have a common folder with code that is the same for both versions.

Next steps

  • Figure out tests to be executed against V4 and V5 (some tests might be specific)
  • Publish process to build & publish both versions at the same time
  • Flow types seem to be identical, any chance deviation will be required in the future?
  • Tests for mixed versions are broken
  • Consolidate tsconfig.json from a separate src/v(4|5) folders

@mweststrate
Copy link
Member

mweststrate commented Jan 9, 2020

@FredyC awesome for picking this up! I think it is greatly gonna help managing the two versions.

As initial thought I would still keep the build, test and source process completely separate. The reason for this is that v4 has (iirc, and otherwise probably will in the future) different browser and compatibility requirements, so the build and TS configuration might very well differ over time, and it would be a shame that v5/6/7 is build with unnecessary polyfills or language restrictions because of v4.

For the same reason I would keep tests apart, so that we can be picky about what we backport and what not. Also it might make developing a feature easier if you can focus on first getting it right in one of the two.

Edit: same for flow types.

Note that MST has also a multi package setup.

I think always releasing both on the same time would be ok by the way, even if it means that one of them occassionally has no actual change.

@danielkcz
Copy link
Contributor Author

danielkcz commented Jan 9, 2020

Edit: same for flow types.

I am not that familiar with Flow and I am not sure how could that be split by version. However, I do think that having the same API layer for both versions and thus the same types is a sort of sanity check as well.

Note that MST has also a multi package setup.

Yea, but that's just a regular Lerna based monorepo. I believe this is something different. I don't even think Lerna allows to publish to the same package with two different versions at once.

I can really imagine over time to have an src/common and src/test folders to contain bits and pieces that will definitely stay the same across versions. It might increase reliability and confidence for the stable core that should not break.

I agree with everything else. Take it slowly, for now, and expand on the idea later. I also want to update the PR template to explain how to contribute to this new structure.

Currently, I am waiting for that new PR to be resolved to avoid painful patching. Then I will jump right into it.

@mweststrate
Copy link
Member

Yea, but that's just a regular Lerna based monorepo. I believe this is something different. I don't even think Lerna allows to publish to the same package with two different versions at once.

I'd just put two packages into one repo for starter, and wire up the scripts. Lerna is probably overkill here and has it's own issues. This is not an n packages problem, but 2 pacakages. So I think things can remain KISS, and we can just call build, test, publish etc in two different directories, and have both packages with a package.json that has the same name. The build setup doesn't change often, so I fear the complexity of tools like lerna more than having slightly longer scripts :)

@coveralls
Copy link

coveralls commented Jan 22, 2020

Coverage Status

Coverage decreased (-0.02%) to 93.417% when pulling 77e18c4 on combine-versions into 11b7c2f on master.

@danielkcz danielkcz marked this pull request as ready for review January 22, 2020 19:12
@danielkcz
Copy link
Contributor Author

danielkcz commented Jan 22, 2020

Alright, I am done here. By tomorrow I will merge this and we can figure out possible issues later. Probably best not to keep this PR open for too long.

Summary of changes

  • master/src -> master/src/v5
  • master/test -> master/test/v5
  • mobx4-master/src -> master/src/v4
  • mobx4-master/test -> master/test/v4
  • root package.json is source of truth for version (minor.patch)
  • build is done for both versions with output to dist/vX folder
  • package.json is copied over to these dist folders with correct major version
  • publish will bump version (patch or minor) in root package.json and in dist as well
  • test/perf modified to run against both versions
  • upgraded size-limit and configured to run on both versions

As for publishing, we agreed with Michel it's fine to keep versions in sync even if there would be a change in either major version only. An updated publishing script does streamline this process.

Generally, tests are working. The "mixed-versions" test is broken completely and I did not want to bother with that now and delay this PR more.

The "webpack-regression" tests are green in CI build, but failing nonetheless. Not sure if it's even needed anymore and what was the purpose of that.

@mweststrate
Copy link
Member

Generally, tests are working. The "mixed-versions" test is broken completely and I did not want to bother with that now and delay this PR more.

Move them over to a separate PR for now in that case?

The "webpack-regression" tests are green in CI build, but failing nonetheless. Not sure if it's even needed anymore and what was the purpose of that.

It's really old, to verify somewhere in the past that we bundled correctly. Feel free to drop it if it is in the way.

One critical issue I found is that version 4 will be published under the latest tag in npm. It should get the the mobx4 tag, so that people do get a 5 version installed always by default.

Beyond that LGTM. So feel free to merge if above things are addressed. Than we can deal with the fallout later.

@danielkcz
Copy link
Contributor Author

Thanks for pointing out about dist-tag, did not realize that, so that's fixed now. Removed webpack tests.

Who knows how long have been the mixed-versions tests broken, it wasn't executing as part of CI. I will open a PR with that added to CI and someone can hopefully look at that later.

@danielkcz danielkcz merged commit 582a516 into master Jan 23, 2020
@mweststrate
Copy link
Member

awesome, thanks for doing 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.

3 participants