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

Change src imports to relative #3082

Merged
merged 11 commits into from
Jul 22, 2021
Merged

Change src imports to relative #3082

merged 11 commits into from
Jul 22, 2021

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Jul 20, 2021

As an alternative to #3074, which adds a clean step to rwfw project:sync to get around the ttypescript error, we can solve the problem by not using ttypescript at all, getting rid of the transform altogether.

Not having the transform makes for way-more awkward relative imports. This isn't super ideal, but it's a change that only affects framework developers. The alternative is migrating to ts-patch, which seems like it'll involve more config than ttypescript (which was basically zero).

Fixes #3054

@jtoar jtoar self-assigned this Jul 20, 2021
@jtoar jtoar changed the title Ds change src imports to relative Change src imports to relative Jul 20, 2021
@thedavidprice thedavidprice requested review from peterp and dac09 July 20, 2021 23:36
@thedavidprice thedavidprice added this to the next-release-priority milestone Jul 20, 2021
@thedavidprice
Copy link
Contributor

Looks like CodeQL is failing on regex. 60 cases in test files. One in the Render setup command.

For render, it's failing on getPaths().base.match(/[^/|\\]+$/)[0], which I believe we use to handle Windows + Posix paths.

For every other case, it's failing on cell.match(/(userProfiles.*?\})/s)[1]

@cannikin do you see a way to resolve these? I'm fine to dismiss them as "used in tests" for the applicable alerts. If there's a better way to handle the paths for Windows + Posix, we should look for other usage in the codebase as well.

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

I love this approach @jtoar, what I would suggest is completely disabling the src alias in babel, searching for 'src/ and making sure that it's completely removed. It's safe to dismiss the CodeQL warnings.

@dac09
Copy link
Contributor

dac09 commented Jul 21, 2021

@jtoar - I've removed the babel plugin, and also all references to src/lib/test.

@peterp - this means we're getting closer to be able to use esbuild within the framework 🚀

@peterp
Copy link
Contributor

peterp commented Jul 21, 2021

As far as I know ESBuild already works with aliases if they're specified in the paths section of a {ts|js}config.json file, just not in "bundle" mode, which is something that I'm on the fence about.

Which reminds me! The src/* paths section should also be removed from {ts|js}config.json.

@dac09
Copy link
Contributor

dac09 commented Jul 21, 2021

Which reminds me! The src/* paths section should also be removed from {ts|js}config.json.

Good point - doing it now

@dac09
Copy link
Contributor

dac09 commented Jul 21, 2021

Will wait for @jtoar to wake up then merge.

TS team high five ✋

@dac09
Copy link
Contributor

dac09 commented Jul 21, 2021

As far as I know ESBuild already works with aliases if they're specified in the paths section of a {ts|js}config.json file, just not in "bundle" mode, which is something that I'm on the fence about.

The problem was that it wasn't transforming the paths last time I tried esbuild - there's an open issue for it on their repo. One for next release or post-v1 to re-investigate I suppose!

@peterp
Copy link
Contributor

peterp commented Jul 21, 2021

It looks like it was closed in May 2020: evanw/esbuild#60, good news all round!

@peterp
Copy link
Contributor

peterp commented Jul 21, 2021

It looks like it only works when you ARE bundling! Thanks for the heads up @dac09

@cannikin
Copy link
Member

@cannikin do you see a way to resolve these?

For the paths one, I would think we could just give the path to the path lib and let it worry about whether to worry about / or \ for us? Then just get the first part of the path from that?

For the second one, we could do a little looser search, and just look for the string "user" all by itself on a line in the whole cell, rather than limiting the check to just the GraphQL query at the top of the cell. This assumes that nowhere else in the page does "user" appear on a line all by itself, which seems reasonable right now.

Try removing line 323 and change 325 to:

expect(cell).not.toMatch(/^\s+user$/m)

@thedavidprice thedavidprice linked an issue Jul 21, 2021 that may be closed by this pull request
@jtoar jtoar force-pushed the ds-change-src-imports-to-relative branch from d30959f to 678172f Compare July 21, 2021 23:49
@jtoar jtoar merged commit f359de4 into main Jul 22, 2021
@thedavidprice thedavidprice deleted the ds-change-src-imports-to-relative branch July 22, 2021 00:45
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.

Framework Build - Typescript Error
6 participants