Add .js file extension to all in-project imports
#10994
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as off-topic.
This comment was marked as off-topic.
size-limit report 📦
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
A new release has been made for this PR. You can install it with |
|
Bundling changes from a TypeScript perspective can be seen here: AreTheTypesWrong result for @apollo/[email protected]
AreTheTypesWrong result for @apollo/[email protected]
=> we're probably not end up "perfect" without an |
phryneas
changed the title
.js file extension to all in-project imports.js file extension to all in-project imports
|
A PR that I made over in attw (arethetypeswrong/arethetypeswrong.github.io#46) now also checks all entry points and just revealed one more wrong import, and now it seems we're as good as we can be: I guess we should add this check to our CI once it's merged? (For comparison, here is the result for AC 3.7) |
|
So we now get AreTheTypesWrong results in the Job Summary. I believe that's good enough and probably better than having yet another bot comment? |
I'm totally fine with this. We've got a decent amount of noise with bot comments right now, so the check seems reasonable to me. |
jerelmiller
reviewed
Jun 27, 2023
jerelmiller
approved these changes
Jun 27, 2023
config/rollup.config.js
Outdated
| @@ -137,7 +163,8 @@ function prepareBundle({ | |||
|
|
|||
| export default [ | |||
| ...entryPoints.map(prepareBundle), | |||
| // Convert the ESM entry point to a single CJS bundle. | |||
| //.filter(x => x.input.includes("utilities")), | |||
There was a problem hiding this comment.
Suggested change
| //.filter(x => x.input.includes("utilities")), |
Do we need this line?
config/rollup.config.js
Outdated
| @@ -109,9 +109,12 @@ function prepareBundle({ | |||
|
|
|||
| return { | |||
| input: inputFile, | |||
| /* | |||
| external(id, parentId) { | |||
There was a problem hiding this comment.
Is this needed anymore? I see its commented out, so perhaps we can just remove it altogether?
There was a problem hiding this comment.
There's quite a bunch of stuff that can be completely removed at this point - also the full postprocessDist.ts - I'm gonna do another cleanup on this :)
config/postprocessDist.ts
Outdated
| }); | ||
| // since we have all the file extensions in the src directory now, we can | ||
| // skip that part | ||
| // and the other PR will remove the __DEV__ check, so this whole file can |
config/rollup.config.js
Outdated
| @@ -137,7 +163,8 @@ function prepareBundle({ | |||
|
|
|||
| export default [ | |||
| ...entryPoints.map(prepareBundle), | |||
| // Convert the ESM entry point to a single CJS bundle. | |||
| //.filter(x => x.input.includes("utilities")), | |||
| // Convert the ESM entry point to a single CJS bundle.# | |||
There was a problem hiding this comment.
Suggested change
| // Convert the ESM entry point to a single CJS bundle.# | |
| // Convert the ESM entry point to a single CJS bundle. |
|
Differences between before this change and after can now be seen in the "Summary" tab of the "Compare Build Output" GitHub action. |
|
Okay, let's get this in :) |
This was referenced Aug 2, 2023
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Triggered by #9890 - I'm not sure yet how to validate that this is not a breaking change in some environments. Ideas welcome.
Checklist: