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

chore: suggestions #856

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

marianfoo
Copy link

Some suggestions, please create a discussion if you need more info

I also created a quick sample for using tenants, for a customer i developed the generic usage of a base.js (the naming is maybe confusing with the base api).

I make it draft because this is not intended to be merged

Copy link

changeset-bot bot commented Sep 13, 2024

⚠️ No Changeset found

Latest commit: 4fa443d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -77,16 +72,16 @@ Each service must provide at least two things
On top of that you can specify one of the following option

- mockdataPath : the path to the folder containing the mockdata files
- generateMockData : whether or not you want to use automatically generated mockdata
- generateMockData : whether or not you want to use automatically generated mockdata <-- TODO: what happens if i set this to false and only define 1 of 2 entites in the service? error in the frontend?
Copy link
Member

Choose a reason for hiding this comment

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

no data in the frontend, if you say generateMockData: false you declare that you will take care of all the mockdata yourself

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense, but i can define 1 out of 3 OData Services with mockdata and 2 out of 3 are coming with proxy from live data, correct?

packages/ui5-middleware-fe-mockserver/README.md Outdated Show resolved Hide resolved
packages/ui5-middleware-fe-mockserver/README.md Outdated Show resolved Hide resolved
samples/basic-usage/ui5.yaml Outdated Show resolved Hide resolved
samples/basic-usage/ui5.yaml Outdated Show resolved Hide resolved
metadataXmlPath: ./webapp/localService/metadata.xml
# TODO: is mockdataPath necessary when folder not defined?
Copy link
Member

Choose a reason for hiding this comment

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

it's necessary as soon as you want mockdata to be found :)

Copy link
Author

Choose a reason for hiding this comment

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

Ok, it may makes sense to add the folder including the config in the yaml to create the structure when you may add mockdata later

samples/error-handling/ui5.yaml Outdated Show resolved Hide resolved
let filePath = '';

// Extract the base file name without extension
const baseFileName = path.basename(fileName, path.extname(fileName)).replace(/\.js$/, '');
Copy link
Member

Choose a reason for hiding this comment

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

I get the idea but for a sample it seems overkill.
I did however, consider including this in the mockserver by default, just have to come up with a proper naming / folder organization for that.

Copy link
Author

Choose a reason for hiding this comment

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

That would be great! Little bit like CAP with convention over configuration.
Ideally you don´t define the js file at all because if i dont use the action my js file all look the same and use the base code.
So in my data folder i only define RootEntity.jsonand RootEntity-100.json and not the js file.
Only if want use custom coding when loading the json file create the js file for the Entity.

@nlunets
Copy link
Member

nlunets commented Sep 18, 2024

Thanks for this nice starting point, I think we can improve the docu there, and i'll look into making the tenant specific mockdata thing part of the builtin functionality

@marianfoo
Copy link
Author

What should we do with the PR? Should i create a different PR for the tenants sample that we can merge when your feature is ready or do you want to create your own sample?
We can use this PR only for the documentation

@nlunets
Copy link
Member

nlunets commented Sep 19, 2024

If you can create a different PR with the sample feature i'll just use that as a base for the auto loading of data :)
And we keep this one for docu yes

@marianfoo
Copy link
Author

If you can create a different PR with the sample feature i'll just use that as a base for the auto loading of data :) And we keep this one for docu yes

created here #859

@marianfoo marianfoo marked this pull request as ready for review September 23, 2024 12:44
@nlunets
Copy link
Member

nlunets commented Nov 14, 2024

Readme specific update are covered in #888

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