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

[SEARCHEXP-1358]Handle relative path and resolve to their real path #164

Merged
merged 7 commits into from
Jul 11, 2023

Conversation

GCpigsic
Copy link

@GCpigsic GCpigsic commented Jun 6, 2023

Description

Ticket: SEARCHEXP-1358
Investigation and decision log: Decision log
For the consumer side, it will fail to compile when importing a typed logic from the common package, which is imported by npm local import

We discussed the proposals listed in this decision log, and we all agree to adopt Option 3 as a medium-term option before new potential production standard carry out. That is 👇:

  • Add a logic to handle the path which is not a part of its node_modules

ℹ️ We will release a beta version first, and verify in CI that this works in other microsites. Once everything goes well, we will release a main version

How to solve it

  • Identify local paths and resolve to their real path

Screenshots

Compile and build successfully in Banana with the changes

arnaugm
arnaugm previously approved these changes Jun 9, 2023
? bpkReactScriptsConfig.babelIncludePrefixes.map(
(prefix) => new RegExp(`node_modules[\\/]${prefix}`)
)
? bpkReactScriptsConfig.babelIncludePrefixes.map(prefix => {
Copy link

Choose a reason for hiding this comment

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

are we able to add unit tests for this at all?

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 a good idea, added.
but seems BRS is lacking unit tests and there is no process in the pipeline to run the unit test automatically(I couldn’t find it if I don’t miss something), in this case, seems adding unit tests for this change doesn’t make much sense. Seems the test facility in BRS is not fully equipped, what do you think 🤔️

@@ -1,6 +1,6 @@
{
"name": "@skyscanner/backpack-react-scripts",
"version": "10.4.0",
"version": "10.4.1",

Choose a reason for hiding this comment

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

This should happen after PR is merged and new version is released.

Copy link
Author

Choose a reason for hiding this comment

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

Good to know, thanks!

@arnaugm
Copy link

arnaugm commented Jul 3, 2023

beta available in https://www.npmjs.com/package/@skyscanner/backpack-react-scripts/v/10.4.1-beta.1

@mungodewar mungodewar merged commit 5610d07 into Skyscanner:fork Jul 11, 2023
mungodewar pushed a commit that referenced this pull request Jul 11, 2023
…164)

* handle relative path

* remove console.log

* handle relative path

* unformat

* update version

* add unit test

* revert change log

---------

Co-authored-by: gc.zhu <gc.zhu@skyscanner.net>
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.

5 participants