Skip to content

Conversation

@mshima
Copy link
Contributor

@mshima mshima commented Jun 21, 2021

Generated esm plugins are generated like plugin/duration/index.js, but import still points to import dayjs from '../index', but it should be import dayjs from '../../index'.

  • Fix imports.
  • Add tests.
  • Update babel to release.

@codecov
Copy link

codecov bot commented Jun 26, 2021

Codecov Report

Merging #1544 (05e1da4) into dev (c5688f3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev     #1544   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          176       177    +1     
  Lines         1980      1986    +6     
  Branches       502       503    +1     
=========================================
+ Hits          1980      1986    +6     
Impacted Files Coverage Δ
src/locale/sv-fi.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5688f3...05e1da4. Read the comment docs.

@@ -0,0 +1,11 @@
import dayjs from 'dayjs/esm';
Copy link
Owner

Choose a reason for hiding this comment

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

what's this file used for, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test esm generated files.

@iamkun
Copy link
Owner

iamkun commented Jun 28, 2021

e.g. in file esm/locale/af.js,import dayjs from '../index' means to import root/esm/index.js, should correct don't we?

@mshima
Copy link
Contributor Author

mshima commented Jun 28, 2021

e.g. in file esm/locale/af.js,import dayjs from '../index' means to import root/esm/index.js, should correct don't we?

Yes, it’s just changing plugin directories.
I think locales folder should be ok.

@iamkun
Copy link
Owner

iamkun commented Jun 29, 2021

e.g. in file esm/locale/af.js,import dayjs from '../index' means to import root/esm/index.js, should correct don't we?

Yes, it’s just changing plugin directories.
I think locales folder should be ok.

seems we don't have to change the path here?

@mshima
Copy link
Contributor Author

mshima commented Jun 29, 2021

seems we don't have to change the path here?

Locale is ok, plug-ins generates a folder due to typings.

@iamkun
Copy link
Owner

iamkun commented Jun 29, 2021

seems we don't have to change the path here?

Locale is ok, plug-ins generates a folder due to typings.

Can you please detail it? Seems there's no plugin has the import statement import ../index.js

@mshima
Copy link
Contributor Author

mshima commented Jun 29, 2021

Honestly I don’t know why Babel adds it.
Look at esm/plugins/*/index.js.

Probably due to typings.

import { OpUnitType, UnitTypeLongPlural } from "../index";

Changing that to dayjs may fix the problem too I will test.

@mshima
Copy link
Contributor Author

mshima commented Jun 29, 2021

The error is just with duration plugin.
Do you want me to drop the tests?

@mshima mshima requested a review from iamkun July 13, 2021 20:47
const filePath = path.join(localeDir, l)
const readFile = await promisify(fs.readFile)(filePath, 'utf8')
const result = readFile.replace("'dayjs'", "'../index'")
const result = readFile.replace(/'dayjs'/g, "'dayjs/esm'")
Copy link
Owner

Choose a reason for hiding this comment

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

seems ../index should be the same file as dayjs/esm, don't it?

Copy link
Contributor Author

@mshima mshima Aug 4, 2021

Choose a reason for hiding this comment

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

Yes, for locale, it’s just to match plugins logic.
Plugins generated structure is different due to typings.
translation/en.js vs plugins/duration/index.js

Copy link
Owner

Choose a reason for hiding this comment

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

You can check the final file here https://unpkg.com/browse/dayjs@1.10.6/esm/locale/af.js

image

../index refers to esm/index.js, which I think it's correct?

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.

2 participants