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

fix(appConfig): Improper path used when importing appConfig #123

Merged
merged 1 commit into from
Apr 22, 2020

Conversation

MarkLark86
Copy link
Contributor

No description provided.

@MarkLark86 MarkLark86 added this to the 1.33.0 milestone Apr 22, 2020
@MarkLark86 MarkLark86 requested a review from tomaskikutis April 22, 2020 07:39
@tomaskikutis
Copy link
Member

How does it know which project and file it has to import the constant from? Why doesn't the full path work?

@MarkLark86
Copy link
Contributor Author

I noticed when I was working on the appConfig in planning that if I used absolute path, the config overrides (i.e. from client_config) wasn't being applied for some reason. Only when I used the import in the same way client-core did the overrides work

@MarkLark86
Copy link
Contributor Author

The webpage resolve module config resolves this reference to the correct file

@tomaskikutis
Copy link
Member

I noticed when I was working on the appConfig in planning that if I used absolute path, the config overrides (i.e. from client_config) wasn't being applied for some reason

It depends on where the config is read. To get client_config an async HTTP request is made and until the application loads(before bootstrapping angular), the values from the server aren't present in appConfig. If you are reading values at the top level of your javascript file, it's synchronous and is called immediately as the file is imported, not waiting for the startup of the app.

I'm still not entirely sure why importing it differently would help. It might be a lucky timing because of the webpack bundling order or that because you are importing it from a different project, you get a fresh new instance of the object which is unpatched with client_config.

@MarkLark86
Copy link
Contributor Author

It depends on where the config is read. To get client_config an async HTTP request is made and until the application loads(before bootstrapping angular), the values from the server aren't present in appConfig. If you are reading values at the top level of your javascript file, it's synchronous and is called immediately as the file is imported, not waiting for the startup of the app.

I made sure not to use the appConfig in any global scope, they should all be within function scopes after the application has been bootstrapped, so I don't believe this was the issue.

I'm still not entirely sure why importing it differently would help. It might be a lucky timing because of the webpack bundling order or that because you are importing it from a different project, you get a fresh new instance of the object which is unpatched with client_config.

I have a theory. It might be because I had superdesk-planning linked locally which had it's own copy of superdesk-client-core in it's node_modules. By using webpack to resolve to the scripts folder, not using absolute importing was making sure the same appConfig file was imported.

@MarkLark86 MarkLark86 merged commit 0eaa11c into superdesk:master Apr 22, 2020
@tomaskikutis
Copy link
Member

It might be because I had superdesk-planning linked locally which had it's own copy of superdesk-client-core

That might be the issue. Why does planning have a dependency on superdesk-core? Shouldn't it be the other way around?

@MarkLark86
Copy link
Contributor Author

That might be the issue. Why does planning have a dependency on superdesk-core? Shouldn't it be the other way around?

Planning imports some features from superdesk-core, and this is placed in the devDependencies. So this is only an issue when locally developing on these two modules

@tomaskikutis
Copy link
Member

I'm wondering if making a peer dependency would make it better, in a way that it would enforce a single instance even in dev mode

MarkLark86 added a commit to MarkLark86/superdesk-analytics that referenced this pull request Apr 22, 2020
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