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

Include src/ directory in npm package to fix source map loading #23

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

tchebb
Copy link
Contributor

@tchebb tchebb commented Jan 25, 2022

Description

The compiled JavaScript we ship includes embedded source maps because we enable "inlineSourceMap" in tsconfig.json. Those source maps reference the original TypeScript files by name instead of embedding their contents because we don't set "inlineSources".

However, we currently don't include those TypeScript files in the package we distribute. We only include the compiled files from build/. This causes a warning when source-map-loader, run from a downstream package, tries to load maps:

WARNING in ./node_modules/plausible-tracker/build/module/index.js
Module Warning (from ./node_modules/source-map-loader/dist/cjs.js):
Failed to parse source map from '.../node_modules/plausible-tracker/src/index.ts' file: Error: ENOENT: no such file or directory, open '.../node_modules/plausible-tracker/src/index.ts'
 @ ./src/analytics/plausible.ts 3:0-42 11:29-38
 @ ./src/index.tsx 8:0-55 13:0-14

To fix the issue, add src/ to "files" in package.json. Alternatively, we could have set "inlineSources" to true in tsconfig.json, but that results in the TypeScript being encoded as Base64, which is less efficient than just shipping the files directly.

Related Issue

#22

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

The compiled JavaScript we ship includes embedded source maps because we
enable "inlineSourceMap" in tsconfig.json. Those source maps reference
the original TypeScript files by name instead of embedding their
contents because we don't set "inlineSources".

However, we currently don't include those TypeScript files in the
package we distribute. We only include the compiled files from build/.
This causes a warning when source-map-loader, run from a downstream
package, tries to load maps:

    WARNING in ./node_modules/plausible-tracker/build/module/index.js
    Module Warning (from ./node_modules/source-map-loader/dist/cjs.js):
    Failed to parse source map from '.../node_modules/plausible-tracker/src/index.ts' file: Error: ENOENT: no such file or directory, open '.../node_modules/plausible-tracker/src/index.ts'
     @ ./src/analytics/plausible.ts 3:0-42 11:29-38
     @ ./src/index.tsx 8:0-55 13:0-14

To fix the issue, add src/ to "files" in package.json. Alternatively, we
could have set "inlineSources" to true in tsconfig.json, but that
results in the TypeScript being encoded as Base64, which is less
efficient than just shipping the files directly.
tchebb added a commit to tchebb/wayland-explorer that referenced this pull request Jan 25, 2022
This makes several significant changes to our dependencies:
 - react-scripts (i.e. create-react-app) is now v5 (from v4)
 - tailwindcss is now v3 (from a variant of v2 targeting postcss v7)
 - postcss is now v8 (from v7)
 - autoprefixer is now v10 (from v9)
 - craco is no longer used because react-scripts v5 natively supports
   tailwindcss

The postcss and autoprefixer upgrades required no changes on our end, as
they're used only through tailwindcss.

The craco removal required minor changes to package.json to go back to
invoking react-scripts directly.

The react-scripts upgrade didn't require any changes apart from the
other dependency upgrades. However, it did introduce a couple new
harmless warnings to the build. The first is a pair of
"DeprecationWarning" messages from webpack-dev-server for which a fix
has already been submitted upstream to create-react-app[1]. The second
is three "Failed to parse source map" messages from source-map-loader.
These are due to a bug in plausible-tracker for which I've submitted a
fix[2].

The tailwindcss upgrade was the most invasive and required a number of
changes to our CSS and config. I fully followed the upgrade guide[3]
and tested as much functionality as I could. As far as I can tell, the
site behaves and appears identical to how it did before.

[1] facebook/create-react-app#11862
[2] plausible/plausible-tracker#23
[3] https://tailwindcss.com/docs/upgrade-guide
@ukutaht
Copy link
Contributor

ukutaht commented Jan 25, 2022

Makes sense, thank you!

@ukutaht ukutaht merged commit ee2cc27 into plausible:master Jan 25, 2022
@ukutaht
Copy link
Contributor

ukutaht commented Jan 25, 2022

Just published v0.3.5 on NPM. Give it a try

@tchebb tchebb deleted the ship-source-files branch January 25, 2022 21:32
tchebb added a commit to tchebb/wayland-explorer that referenced this pull request Jan 26, 2022
This makes several significant changes to our dependencies:
 - react-scripts (i.e. create-react-app) is now v5 (from v4)
 - tailwindcss is now v3 (from a variant of v2 targeting postcss v7)
 - postcss is now v8 (from v7)
 - autoprefixer is now v10 (from v9)
 - craco is no longer used because react-scripts v5 natively supports
   tailwindcss

The postcss and autoprefixer upgrades required no changes on our end, as
they're used only through tailwindcss.

The craco removal required minor changes to package.json to go back to
invoking react-scripts directly.

The react-scripts upgrade didn't require any changes apart from the
other dependency upgrades. However, it did introduce a couple new
harmless warnings to the build. The first is a pair of
"DeprecationWarning" messages from webpack-dev-server for which a fix
has already been submitted upstream to create-react-app[1]. The second
is three "Failed to parse source map" messages from source-map-loader.
These are due to a bug in plausible-tracker for which I've submitted a
fix[2].

The tailwindcss upgrade was the most invasive and required a number of
changes to our CSS and config. I fully followed the upgrade guide[3]
and tested as much functionality as I could. As far as I can tell, the
site behaves and appears identical to how it did before.

[1] facebook/create-react-app#11862
[2] plausible/plausible-tracker#23
[3] https://tailwindcss.com/docs/upgrade-guide
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.

None yet

2 participants