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

feat(arch): add support new arch #422

Merged
merged 2 commits into from
May 31, 2024

Conversation

ngocle2497
Copy link

Add support new arch for android + ios

@ngocle2497
Copy link
Author

@rajdeepnanua-okta can u review this PR?

@rajdeepnanua-okta
Copy link
Contributor

rajdeepnanua-okta commented May 9, 2024

Hi @ngocle2497, thanks for contributing this PR! It will take some time for me to review this PR given the size. But I have a few comments/questions:

  1. I see that you removed e2e test app, and added example app. Did the e2e app have issues with these changes? If not, I would prefer e2e app to not be removed. If there were issues here, I'd like to understand what they are.
  2. Similarly, files under script folder shouldn't be removed either. There are some files there used in our internal build system.
  3. test/index.test.js was removed. This is a necessary file for our JS unit tests, and this PR should ensure those tests are still passing. I am okay with moving this file to a different directory, as long as the tests are still there and we can run them.

I will review this in more details soon. But after a quick glance at your PR, everything else seems to be looking good. Thanks again for your contribution!

@ngocle2497
Copy link
Author

Hi @ngocle2497, thanks for contributing this PR! It will take some time for me to review this PR given the size. But I have a few comments/questions:

  1. I see that you removed e2e test app, and added example app. Did the e2e app have issues with these changes? If not, I would prefer e2e app to not be removed. If there were issues here, I'd like to understand what they are.
  2. Similarly, files under script folder shouldn't be removed either. There are some files there used in our internal build system.
  3. test/index.test.js was removed. This is a necessary file for our JS unit tests, and this PR should ensure those tests are still passing. I am okay with moving this file to a different directory, as long as the tests are still there and we can run them.

I will review this in more details soon. But after a quick glance at your PR, everything else seems to be looking good. Thanks again for your contribution!

  1. During the migration, I recreated the library instead of migrating (it made the migration easier), so example is the default directory.
    Furthermore, e2e is a javascript template. Typescript has been the default template for quite a while so I thought it would be a good idea to upgrade to avoid confusion or difficulties when running the example project.
  2. brought it back
  3. brought it back

@rajdeepnanua-okta rajdeepnanua-okta changed the base branch from master to new_arch May 31, 2024 15:42
@rajdeepnanua-okta rajdeepnanua-okta merged commit a7e3bb7 into okta:new_arch May 31, 2024
@rajdeepnanua-okta
Copy link
Contributor

Hi @ngocle2497, I've merged your changes into a local feature branch. I'm going to do a couple sanity checks to make sure everything is working properly, and will then release the changes into master. Thanks again for the changes!

@shaneboyar
Copy link

@rajdeepnanua-okta as far as I can tell this change hasnt been included in a new release yet, do you have any insight into when that will be published?

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