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

CSF3: Normalize windows paths in autoTitle #15770

Merged

Conversation

yngvebn
Copy link
Contributor

@yngvebn yngvebn commented Aug 5, 2021

Issue:

What I did

In order for autoTitle to work on windows, the paths needs to be normalized into unix-style paths

How to test

  • Is this testable with Jest or Chromatic screenshots? Yes
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Aug 5, 2021

Nx Cloud Report

CI ran the following commands for commit 98f5aa0. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@shilman shilman changed the title Core-Client: Normalize paths when attempting to create autoTitle CSF3: Normalize windows paths when attempting to create autoTitle Aug 5, 2021
@yngvebn yngvebn force-pushed the normalize_autotitle_path_for_windows branch from 8647675 to 8b98db3 Compare August 6, 2021 06:04
@shilman
Copy link
Member

shilman commented Aug 6, 2021

@yngvebn there's some problem with IE in this PR. is that something you can debug? cc @ndelangen @tooppaaa

https://www.chromatic.com/build?appId=5def62c97bd45f0020ef370e&number=9053

@yngvebn
Copy link
Contributor Author

yngvebn commented Aug 6, 2021

@yngvebn there's some problem with IE in this PR. is that something you can debug? cc @ndelangen @tooppaaa

https://www.chromatic.com/build?appId=5def62c97bd45f0020ef370e&number=9053

Let me have a look

@yannbf yannbf requested a review from ghengeveld August 9, 2021 15:01
@yannbf yannbf self-assigned this Aug 9, 2021
@yngvebn
Copy link
Contributor Author

yngvebn commented Aug 12, 2021

A recap from discussions on Discord:

I've added a tiny package (https://www.npmjs.com/package/slash) for normalizing paths. At first I added the most recent version (4). This was a pure ESM module, and all the jest tests failed with ESM related errors. Upgrading to an older version made those errors go away, but now there seems to be problems with the bundling of this module:

So, I couldn't understand why this was happening on my branch... Seems the IE problem is not only on my branch... I checked a couple of the other netlify urls from different PRs, and they all fail with the same errors... :/
it does seem like it's related to some Angular modules not being transpiled down for IE11
However, for the cra-ts-essentials storybook, it's once again transpiling of slash that's missing
So what's odd, is that the vendor.js for cra-ts-essentials is not transpiling the slash-library, but the vendor.js for the angular-cli does.

// Angular cli vendor.js:
function(module, exports, __webpack_require__) {
    "use strict"; __webpack_require__("../../node_modules/core-js/modules/es.string.replace.js"), __webpack_require__("../../node_modules/core-js/modules/es.regexp.exec.js"),
        module.exports = function (path) {
            var isExtendedLengthPath = /^\\\\\?\\/.test(path),
                hasNonAscii = /[^\u0000-\u0080]+/.test(path);
            return isExtendedLengthPath || hasNonAscii ? path : path.replace(/\\/g, "/")
        }
}

// cra-ts-essential vendor.js:

function(module, exports, __webpack_require__) {
    "use strict";
    module.exports = path => {
        const isExtendedLengthPath = /^\\\\\?\\/.test(path),
            hasNonAscii = /[^\u0000-\u0080]+/.test(path);
        
        return isExtendedLengthPath || hasNonAscii ? path : path.replace(/\\/g, "/")
    }
}

so, on the latter, IE is obviously kicking it on the path => arrow function

@yngvebn yngvebn force-pushed the normalize_autotitle_path_for_windows branch from b7f8b32 to bb0891d Compare August 13, 2021 06:12
@tooppaaa
Copy link
Contributor

Hey @yngvebn, sorry about the delay.
I pushed a fix for es6Transpiler on your branch. Can I let you take over on your actual fix ? :)

@shilman, I'll update addon-ie11 as well

@yngvebn
Copy link
Contributor Author

yngvebn commented Aug 22, 2021

Hey @yngvebn, sorry about the delay.

I pushed a fix for es6Transpiler on your branch. Can I let you take over on your actual fix ? :)

@shilman, I'll update addon-ie11 as well

Amazing, thanks! Yeah, I'll get the rest sorted 👍😊

@yngvebn
Copy link
Contributor Author

yngvebn commented Aug 22, 2021

@tooppaaa is the addon-ie11 the reason for the failing IE11 stories?

@tooppaaa
Copy link
Contributor

I'll have a look.

@tooppaaa
Copy link
Contributor

Looks like IE 11 is back on track. I'm not sure about the failing unit test though

@shilman
Copy link
Member

shilman commented Aug 23, 2021

Thanks @tooppaaa !! And thanks @yngvebn for putting this together!!

@shilman shilman changed the title CSF3: Normalize windows paths when attempting to create autoTitle CSF3: Normalize windows paths in autoTitle Aug 23, 2021
@shilman shilman added this to the 6.4 PRs milestone Aug 23, 2021
@shilman shilman merged commit dc994aa into storybookjs:next Aug 23, 2021
@yngvebn
Copy link
Contributor Author

yngvebn commented Aug 23, 2021

Looks like IE 11 is back on track. I'm not sure about the failing unit test though

Thank you! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants