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.

Sign up for GitHub

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

Jump to bottom

bug: typescript imports using windows paths #4543

Closed
3 tasks done
mrginglymus opened this issue Jul 4, 2023 · 7 comments
Closed
3 tasks done

bug: typescript imports using windows paths #4543

mrginglymus opened this issue Jul 4, 2023 · 7 comments
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@mrginglymus
Copy link

Prerequisites

Stencil Version

4.0.1

Current Behavior

On Windows, the dist-custom-elements target generates a dist/components/index.d.ts with some unescaped windows paths:

/* stencil custom elements */
export { Icon as VIcon } from '..\types\components\icons\icon';
export { defineCustomElement as defineCustomElementVIcon } from './v-icon';

Expected Behavior

/* stencil custom elements */
export { Icon as VIcon } from '../types/components/icons/icon';
export { defineCustomElement as defineCustomElementVIcon } from './v-icon';

System Info

System: node 16.20.0
    Platform: windows (10.0.19045)
   CPU Model: Intel(R) Xeon(R) Gold 5220 CPU @ 2.20GHz (8 cpus)
    Compiler: ...\playground\node_modules\.store\@stencil-core-npm-4.0.1-81248f31a7\node_modules\@stencil\core\compiler\stencil.js
       Build: 1687953951
     Stencil: 4.0.1
  TypeScript: 5.0.4
      Rollup: 2.42.3
      Parse5: 7.1.2
      Sizzle: 2.42.3
      Terser: 5.18.1

Steps to Reproduce

On Windows, the dist-custom-elements target generates a dist/components/index.d.ts with some unescaped windows paths

Code Reproduction URL

n/a

Additional Information

No response
@ionitron-bot ionitron-bot bot added the triage label Jul 4, 2023
@mrginglymus mrginglymus mentioned this issue Jul 4, 2023
Closed
2 tasks
@mrginglymus mrginglymus changed the title bug: bug: typescript imports using windows paths Jul 4, 2023
@alicewriteswrongs
Copy link
Contributor

@mrginglymus thanks for filing this issue, we recently made a change (in Stencil 4) that changed how paths are normalized in Stencil which is responsible for this regression. I'll take a look at your PR and get this addressed!

Thanks again for filing 🙂

@alicewriteswrongs alicewriteswrongs added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label Jul 5, 2023
@ionitron-bot ionitron-bot bot removed the triage label Jul 5, 2023
@alicewriteswrongs alicewriteswrongs mentioned this issue Jul 5, 2023
Merged
2 tasks
alicewriteswrongs added a commit that referenced this issue Jul 5, 2023
@alicewriteswrongs
TEST should fail on windows
51327eb 
In #4317 as part of removing in-browser compilation support we removed
the polyfills for nodejs built-in modules like `path` which were
injected by Rollup during build-time. Although the _main_ purpose of
these polyfills was allowing Stencil to run in the browser in the case
of the `path` module there was also a secondary purpose which was
ensuring that paths were treated the same way across supported platforms
(posix + windows).

See, for instance, the following lines in the polyfill:

https://github.com/ionic-team/stencil/blob/b911f1986a0d583bd1e3cd42cbbca9b255c32f2d/src/compiler/sys/modules/path.ts#L35-L38

These functions basically wrapped the existing path implementation with
our `normalizePath` helper, which would ensure that the output paths
would be the same on both windows and posix systems (e.g. macOS and
linux).

When we merged #4317 an effort was made to add `normalizePath` around
the codebase where it was thought that various paths being calculated
needed to be platform-independent, however, a few locations were missed
(in particular, some paths output into typedefs, which would show up as
non-posix paths when building on windows).

To address the issue we introduce `relative` and `join` functions into
the existing path-related `utils` file (which is incidentally renamed)
which work similarly to how the patched functions in the old polyfill
did. Then several usage sites are changed to import those new utils
instead of the 'raw' functions from `path`. Together these changes
should ensure that Stencil's output is not platform-dependent.

See here for an issue reporting the problem: #4543
@alicewriteswrongs
Copy link
Contributor

@mrginglymus unfortunately I don't have a windows machine handy for testing so I have to rely on our windows CI but I think I have a PR up (#4545) which will address the issue somewhat comprehensively. Is there any chance you could try running a dev build of Stencil to see if it fixes the issue for you?

If so, you can install it like this:

npm install --save-dev @stencil/[email protected]

@mrginglymus
Copy link
Author

Ah, that looks amazing, thanks so much for the quick and comprehensive response. I'll try and test this afternoon.

@mrginglymus
Copy link
Author

Yes, that build has worked perfectly for me :)

@alicewriteswrongs
Copy link
Contributor

Excellent, thanks for taking the time to test that out! I'll get that PR reviewed and merged in soon and it will be in the next release if all goes according to plan.

alicewriteswrongs added a commit that referenced this issue Jul 13, 2023
@alicewriteswrongs
TEST should fail on windows
81d1841 
In #4317 as part of removing in-browser compilation support we removed
the polyfills for nodejs built-in modules like `path` which were
injected by Rollup during build-time. Although the _main_ purpose of
these polyfills was allowing Stencil to run in the browser in the case
of the `path` module there was also a secondary purpose which was
ensuring that paths were treated the same way across supported platforms
(posix + windows).

See, for instance, the following lines in the polyfill:

https://github.com/ionic-team/stencil/blob/b911f1986a0d583bd1e3cd42cbbca9b255c32f2d/src/compiler/sys/modules/path.ts#L35-L38

These functions basically wrapped the existing path implementation with
our `normalizePath` helper, which would ensure that the output paths
would be the same on both windows and posix systems (e.g. macOS and
linux).

When we merged #4317 an effort was made to add `normalizePath` around
the codebase where it was thought that various paths being calculated
needed to be platform-independent, however, a few locations were missed
(in particular, some paths output into typedefs, which would show up as
non-posix paths when building on windows).

To address the issue we introduce `relative` and `join` functions into
the existing path-related `utils` file (which is incidentally renamed)
which work similarly to how the patched functions in the old polyfill
did. Then several usage sites are changed to import those new utils
instead of the 'raw' functions from `path`. Together these changes
should ensure that Stencil's output is not platform-dependent.

See here for an issue reporting the problem: #4543
alicewriteswrongs added a commit that referenced this issue Jul 14, 2023
@alicewriteswrongs
fix(output-targets): fix path normalization logic
f3d6a40 
In #4317 as part of removing in-browser compilation support we removed
the polyfills for nodejs built-in modules like `path` which were
injected by Rollup during build-time. Although the _main_ purpose of
these polyfills was allowing Stencil to run in the browser in the case
of the `path` module there was also a secondary purpose which was
ensuring that paths were treated the same way across supported platforms
(posix + windows).

See, for instance, the following lines in the polyfill:

https://github.com/ionic-team/stencil/blob/b911f1986a0d583bd1e3cd42cbbca9b255c32f2d/src/compiler/sys/modules/path.ts#L35-L38

These functions basically wrapped the existing path implementation with
our `normalizePath` helper, which would ensure that the output paths
would be the same on both windows and posix systems (e.g. macOS and
linux).

When we merged #4317 an effort was made to add `normalizePath` around
the codebase where it was thought that various paths being calculated
needed to be platform-independent, however, a few locations were missed
(in particular, some paths output into typedefs, which would show up as
non-posix paths when building on windows).

To address the issue we introduce `relative` and `join` functions into
the existing path-related `utils` file (which is incidentally renamed)
which work similarly to how the patched functions in the old polyfill
did. Then several usage sites are changed to import those new utils
instead of the 'raw' functions from `path`. Together these changes
should ensure that Stencil's output is not platform-dependent.

See here for an issue reporting the problem: #4543
@alicewriteswrongs
Copy link
Contributor

Just merged #4545, so it will be included in the next Stencil release. Thanks for reporting @mrginglymus!

github-merge-queue bot pushed a commit that referenced this issue Jul 14, 2023
@alicewriteswrongs
fix(output-targets): fix path normalization logic (#4545)
cd5849c 
In #4317 as part of removing in-browser compilation support we removed
the polyfills for nodejs built-in modules like `path` which were
injected by Rollup during build-time. Although the _main_ purpose of
these polyfills was allowing Stencil to run in the browser in the case
of the `path` module there was also a secondary purpose which was
ensuring that paths were treated the same way across supported platforms
(posix + windows).

See, for instance, the following lines in the polyfill:

https://github.com/ionic-team/stencil/blob/b911f1986a0d583bd1e3cd42cbbca9b255c32f2d/src/compiler/sys/modules/path.ts#L35-L38

These functions basically wrapped the existing path implementation with
our `normalizePath` helper, which would ensure that the output paths
would be the same on both windows and posix systems (e.g. macOS and
linux).

When we merged #4317 an effort was made to add `normalizePath` around
the codebase where it was thought that various paths being calculated
needed to be platform-independent, however, a few locations were missed
(in particular, some paths output into typedefs, which would show up as
non-posix paths when building on windows).

To address the issue we introduce `relative` and `join` functions into
the existing path-related `utils` file (which is incidentally renamed)
which work similarly to how the patched functions in the old polyfill
did. Then several usage sites are changed to import those new utils
instead of the 'raw' functions from `path`. Together these changes
should ensure that Stencil's output is not platform-dependent.

See here for an issue reporting the problem: #4543
@rwaskiewicz
Copy link
Contributor

The fix for this issue has been released as a part of today's Stencil v4.0.2 release. As a result, I'm going to close this issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mrginglymus @rwaskiewicz @alicewriteswrongs