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: #8622 #8637

Merged
merged 5 commits into from Jun 18, 2020
Merged

fix: #8622 #8637

merged 5 commits into from Jun 18, 2020

Conversation

CatchABus
Copy link
Contributor

@CatchABus CatchABus commented Jun 14, 2020

PR Checklist

What is the current behavior?

Live-sync fails to load script and style files . (e.g. js, css) for [.]*.xml files.

What is the new behavior?

Reload will work.

Closes #8622 .

This way of making module-resolve reload these files is quite dirty, so I hope there'll be a better alternative of extracting qualifiers from file paths.

@cla-bot cla-bot bot added the cla: yes label Jun 14, 2020
@NathanWalker
Copy link
Contributor

Thanks @dimitrisrk 👍 - we'll wanna check against Angular and Vue projects to make sure doesn't cause any side effects.

@CatchABus
Copy link
Contributor Author

CatchABus commented Jun 14, 2020

Thanks @dimitrisrk 👍 - we'll wanna check against Angular and Vue projects to make sure doesn't cause any side effects.

@NathanWalker Considering Angular filename formats, I suspect there might really be issues since my patch extracts qualifiers by splitting path, using dot "." as separator. At the same time, I'm working on finding a more accurate solution.

EDITED: By the way, according to NS docs, angular does not support all qualifiers:
image

Anyway, let me see if I manage to improve this code more. Thanks.

@CatchABus
Copy link
Contributor Author

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Jun 15, 2020

The cla-bot has been summoned, and re-checked this pull request!

@CatchABus
Copy link
Contributor Author

CatchABus commented Jun 15, 2020

@NathanWalker After few tests of mine with angular and vue app samples, it seems that this module resolver isn't being used for these frameworks's components, but only for NS Core modules.

Either way, I worked on qualifier-matcher and used regular expressions in certain cases.
This way, a great accuracy is achieved in retrieving qualifiers from file names.

Example:

Before this update:
home-page.component.minWH360.js -> Qualifier-matcher will consider 'component' to be a qualifier that does not meet conditions and ignore this file.

After this update:
home-page.component.minWH360.js -> Qualifier-matcher will only retrieve 'minWH360' qualifier using a regular expression and if condition is met, file will be (re)loaded.

This update also fixes #8622 .
Of course, if this is too complicated and unwanted, there is always the option of first commit with split and join.

Please feel free to make tests in depth.

@NathanWalker
Copy link
Contributor

Thanks @dimitrisrk, really appreciate the effort here. Possible to add just a sample component to the e2e/ui-tests-app? This will help lock the desired behavior in to test against as well as help keep track of any possible regressions that could be affected (sounds like none but to be sure).

@NathanWalker NathanWalker self-assigned this Jun 15, 2020
@CatchABus
Copy link
Contributor Author

@NathanWalker Done. Hope this sample is helpful.

CatchABus and others added 4 commits June 16, 2020 12:55
A dirty hack for taking care qualifier-based pages issues during hot module reload.
In the case of screen-size qualifiers, regular expressions are used to ensure proper values.
This commit also includes fix for #8622
@NathanWalker
Copy link
Contributor

Thanks for another excellent contribution @dimitrisrk - this will go out with another round of updates at end of week 👍

@NathanWalker NathanWalker merged commit 4f64bac into NativeScript:master Jun 18, 2020
@NathanWalker NathanWalker added this to Closed in @nativescript/core via automation Jun 18, 2020
@CatchABus
Copy link
Contributor Author

Thanks for another excellent contribution @dimitrisrk - this will go out with another round of updates at end of week 👍

Sounds great. By the way, it seems I missed a semicolon at the end of https://github.com/NativeScript/NativeScript/blob/master/nativescript-core/module-name-resolver/qualifier-matcher/qualifier-matcher.d.ts. Since it's at the end of the file it's causing no issues, I just wanted to point out that small omission.

@NathanWalker
Copy link
Contributor

@dimitrisrk one thing I noticed is that the unit tests fail with these changes. Although it's merged we may hold on releasing it until the unit tests pass. To run them do the following from root of repo:

cd tests
tns run ios --emulator

The results of the tests will output in the simluator and in console and note that most all the failures are from the qualifier matchers. Not sure if tests need to be updated or if this indicates these changes may have introduced regression. Can you give it a shot and if you see same issue you might check and can submit another PR to clear 👍

@CatchABus CatchABus mentioned this pull request Jun 19, 2020
5 tasks
@CatchABus
Copy link
Contributor Author

@NathanWalker I apologize for the inconvenience. It seems there were things I missed. I submitted PR #8658 .
Its latest 4 commits are additions supposed to solve pending problems. Thanks to these additions, I ran unit tests on my computer without issues. If by any chance you still get failures, please let me know.

NathanWalker pushed a commit that referenced this pull request Jun 19, 2020
Co-authored-by: Dimitris - Rafail Katsampas <katsampasdr@gmail.com>
NathanWalker pushed a commit that referenced this pull request Aug 7, 2020
closes #8622

Co-authored-by: Dimitris - Rafail Katsampas <katsampasdr@gmail.com>
NathanWalker pushed a commit that referenced this pull request Aug 7, 2020
Co-authored-by: Dimitris - Rafail Katsampas <katsampasdr@gmail.com>
NathanWalker pushed a commit that referenced this pull request Aug 7, 2020
closes #8622

Co-authored-by: Dimitris - Rafail Katsampas <katsampasdr@gmail.com>
NathanWalker pushed a commit that referenced this pull request Aug 7, 2020
Co-authored-by: Dimitris - Rafail Katsampas <katsampasdr@gmail.com>
tarunama pushed a commit to tarunama/NativeScript that referenced this pull request Sep 9, 2020
…ript#8637)

closes NativeScript#8622

Co-authored-by: Dimitris - Rafail Katsampas <katsampasdr@gmail.com>
tarunama pushed a commit to tarunama/NativeScript that referenced this pull request Sep 9, 2020
Co-authored-by: Dimitris - Rafail Katsampas <katsampasdr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[UPDATED] Live-Sync & page.minWH#.xml problem.
2 participants